refactor: prod ready applies

[pedronauck]

Summary by CodeRabbit

• New Features

  • Expose diagnostics for bridges, skills, and MCP servers (verification, warnings, recovery actions)
  • Provider native-CLI auth support with detailed status and login command/environ
  • MCP server runtime probing showing readiness and tool counts

• Improvements

  • Stronger, more actionable provider error guidance and daemon-unavailable messaging
  • Hardened config/key file and runtime directory permissions
  • Broader secret redaction across transcripts and tool output
  • Release notes/footer augmented with verification posture and provenance guidance

• Bug Fixes

  • Improved scheduler task lease recovery and hermetic test environment behavior

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agh-site	Ready	Preview, Comment	May 20, 2026 2:50pm

[coderabbitai]

Walkthrough

Adds provider/bridge/skill diagnostics, native-CLI auth status, MCP runtime probing, transcript/tool redaction, model canonicalization, CLI/daemon enhancements, permission hardening, and extensive tests/spec/install/footer updates.

Changes

Diagnostics, auth surfaces, and hardening

Layer / File(s)	Summary
End-to-end diagnostics, auth, redaction, and wiring internal/*, packages/site/*, web/src/lib/__tests__/*, .goreleaser.release-footer.md.tmpl, .claudeignore	Implements provider/bridge/skill diagnostics models and API contracts, native-CLI auth/env reporting, MCP runtime status, transcript/tool redaction, CLI/daemon wiring, model alias canonicalization, installer footer/TTY detection, and comprehensive tests/spec updates.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• compozy/agh#21 — Overlaps .claude/.resources ignore and evidence artifact handling.
• compozy/agh#69 — Related changes touching failure-summary construction in internal/acp/failure.go.
• compozy/agh#17 — Prior bridge contract work connected to bridge diagnostics additions.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (3)

internal/api/core/provider_auth_status_test.go (1)

93-98: ⚡ Quick win

Assert the new native-CLI fields directly in this test.

Please also assert AuthStatus.NativeCLI.Present == false and AuthStatus.NativeCLI.Source == "auth_login_command" so regressions on the new auth diagnostics surface are caught.

🤖 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/api/core/provider_auth_status_test.go` around lines 93 - 98, The
test currently checks NativeCLI non-nil and Command but misses asserting the new
fields; update the assertions in provider_auth_status_test.go to also verify
authStatus.NativeCLI.Present == false and authStatus.NativeCLI.Source ==
"auth_login_command" (in addition to the existing Command == "codex") and keep
the existing LoginEnv assertion; reference the authStatus variable and its
NativeCLI field when adding these checks so regressions in the new auth
diagnostic fields are caught.

internal/transcript/transcript.go (1)

203-216: 💤 Low value

Redundant flush when marker text is empty.

When markerText is empty, flushAssistantBuffer is called at line 206, then the function returns at line 208. When markerText is non-empty, another flushAssistantBuffer is called at line 209. The flush at line 209 is always needed for non-empty markers, but the structure could be cleaner.

♻️ Optional simplification

 func appendMarkerTranscriptMessage(messages *[]Message, assistant *assistantBuffer, parsed event) {
 	markerText := transcriptMarkerText(parsed)
+	flushAssistantBuffer(messages, assistant)
 	if markerText == "" {
-		flushAssistantBuffer(messages, assistant)
 		return
 	}
-	flushAssistantBuffer(messages, assistant)
 	*messages = append(*messages, Message{
 		ID:        parsed.ID,
 		Role:      RoleSystem,
 		Content:   markerText,
 		Timestamp: parsed.Timestamp,
 	})
 }

🤖 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/transcript/transcript.go` around lines 203 - 216, The function
appendMarkerTranscriptMessage currently calls flushAssistantBuffer twice;
compute markerText via transcriptMarkerText(parsed), then call
flushAssistantBuffer(messages, assistant) only once before the empty check, and
if markerText=="" return; otherwise append the Message (using parsed.ID,
RoleSystem, markerText, parsed.Timestamp). Update appendMarkerTranscriptMessage
to remove the redundant flush and ensure a single flush occurs for both empty
and non-empty marker cases.

internal/transcript/redaction.go (1)

141-174: 💤 Low value

In-place mutation of unmarshaled JSON value.

redactJSONDisplayValue mutates the input map[string]any and []any in place while also returning them. Since the caller in redactRawMessage unmarshal into a fresh any variable and immediately re-marshals the result, this works correctly. However, if this function were reused elsewhere with a shared value, the mutation would affect the original. The current usage is safe, but consider documenting this behavior or cloning the input for defensive coding.

🤖 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/transcript/redaction.go` around lines 141 - 174,
redactJSONDisplayValue currently mutates map[string]any and []any inputs in
place which can unexpectedly modify shared data; update redactJSONDisplayValue
to avoid in-place mutation by deep-cloning maps and slices before writing (or
explicitly document the in-place behavior) so callers like redactRawMessage
remain safe; specifically, when handling case map[string]any and case []any,
allocate new maps/slices, copy keys/elements, apply redaction into the new copy,
and return that copy (refer to function redactJSONDisplayValue and its callers
such as redactRawMessage, and preserve behavior of sensitiveDisplayJSONField and
redactDisplayString).

🤖 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 `@internal/api/core/bridge_diagnostics.go`:
- Around line 40-44: The diagnostics path currently calls
bridgeProviderForInstance (which invokes ListProviders) for each instance,
causing N instances ⇒ N catalog calls; instead, compute the provider catalog
once and reuse it when building diagnostics: change the diagnostics builder to
accept a pre-fetched provider catalog/map (or memoize ListProviders) and look up
the provider info per instance rather than calling bridgeProviderForInstance
repeatedly; update usages around bridgeSecretBindingsForDiagnostics and the code
paths referenced (the blocks at and around the calls to
bridgeProviderForInstance and bridgeSecretBindingsForDiagnostics) to use the
shared catalog/provider map so ListProviders is invoked only once.

In `@internal/api/core/bridges.go`:
- Around line 75-79: The handler currently aborts with h.respondError and return
when h.bridgeHealthPayloadForInstance fails; change this to degrade gracefully
by catching the error, logging it (or using h.logger if available) and
continuing without returning — set enrichedHealth to the basic health value (or
nil/empty diagnostics) so the rest of the response can be built and returned;
specifically update the error branch around the call to
h.bridgeHealthPayloadForInstance so it does not call h.respondError/return but
instead records the error and proceeds using the original health data in
enrichedHealth.

In `@internal/bridges/diagnostics.go`:
- Around line 115-117: The code is directly embedding provider error/degradation
strings into BridgeDiagnostic.Message (e.g., provider.HealthMessage,
provider.DegradationMessage and other error strings); replace that by first
passing those strings through a redaction helper (e.g., redactSensitiveTokens(s
string)) that strips/replaces claim_token patterns (agh_claim_*), MCP auth
tokens, OAuth codes, PKCE verifiers, secret bindings and other credential-like
patterns with a fixed "<redacted>" placeholder, then use the sanitized result
when building the message and when setting BridgeDiagnostic.Message; add and
reuse this helper wherever provider.* messages or error strings are appended
(refer to BridgeDiagnostic.Message, provider.HealthMessage and
provider.DegradationMessage to locate places to change). Ensure tests or
examples cover typical token patterns to verify redaction.

In `@internal/daemon/settings.go`:
- Line 28: Replace the hardcoded constant settingsMCPProbeTimeout with a
configurable value sourced from config/env/CLI: add a configuration field (e.g.,
MCPProbeTimeout) to your daemon settings struct and load it from config.toml or
an environment variable (with CLI flag override), parse it as a time.Duration,
and use that field wherever settingsMCPProbeTimeout is referenced (including any
duplicate uses), providing a sensible default of 5s if not set; ensure code
refers to the struct field (e.g., cfg.Daemon.MCPProbeTimeout) instead of the old
constant and validate the parsed duration (>0) before use.

In `@internal/settings/models.go`:
- Around line 528-530: cloneProviderItem currently doesn't deep-copy the new
mutable fields on ProviderAuthStatus (LoginEnv and NativeCLI), which can cause
shared-state leaks; update cloneProviderItem to perform a deep copy of LoginEnv
(allocate a new []string and copy elements) and deep-copy NativeCLI by creating
a new ProviderNativeCLIStatus and copying its fields (or cloning via its own
copy helper) before assigning to the cloned ProviderAuthStatus so the returned
struct has independent mutable sub-objects.

---

Nitpick comments:
In `@internal/api/core/provider_auth_status_test.go`:
- Around line 93-98: The test currently checks NativeCLI non-nil and Command but
misses asserting the new fields; update the assertions in
provider_auth_status_test.go to also verify authStatus.NativeCLI.Present ==
false and authStatus.NativeCLI.Source == "auth_login_command" (in addition to
the existing Command == "codex") and keep the existing LoginEnv assertion;
reference the authStatus variable and its NativeCLI field when adding these
checks so regressions in the new auth diagnostic fields are caught.

In `@internal/transcript/redaction.go`:
- Around line 141-174: redactJSONDisplayValue currently mutates map[string]any
and []any inputs in place which can unexpectedly modify shared data; update
redactJSONDisplayValue to avoid in-place mutation by deep-cloning maps and
slices before writing (or explicitly document the in-place behavior) so callers
like redactRawMessage remain safe; specifically, when handling case
map[string]any and case []any, allocate new maps/slices, copy keys/elements,
apply redaction into the new copy, and return that copy (refer to function
redactJSONDisplayValue and its callers such as redactRawMessage, and preserve
behavior of sensitiveDisplayJSONField and redactDisplayString).

In `@internal/transcript/transcript.go`:
- Around line 203-216: The function appendMarkerTranscriptMessage currently
calls flushAssistantBuffer twice; compute markerText via
transcriptMarkerText(parsed), then call flushAssistantBuffer(messages,
assistant) only once before the empty check, and if markerText=="" return;
otherwise append the Message (using parsed.ID, RoleSystem, markerText,
parsed.Timestamp). Update appendMarkerTranscriptMessage to remove the redundant
flush and ensure a single flush occurs for both empty and non-empty marker
cases.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 30a361fe-f0c8-4c02-988a-8afa86a15a53

📥 Commits

Reviewing files that changed from the base of the PR and between c170dd6 and cc5821e.

⛔ Files ignored due to path filters (17)

• .agents/skills/agent-exploration/SKILL.md is excluded by !**/*.md, !.agents/**
• .agents/skills/agent-exploration/assets/AGENT.md is excluded by !**/*.md, !.agents/**
• .agents/skills/agent-exploration/assets/explorer-agent.md is excluded by !**/*.md, !.agents/**
• .agents/skills/agent-exploration/assets/explorer-agent.toml is excluded by !**/*.toml, !.agents/**
• .agents/skills/agent-exploration/references/checklist.md is excluded by !**/*.md, !.agents/**
• .agents/skills/agent-exploration/references/dispatch-rules.md is excluded by !**/*.md, !.agents/**
• .agents/skills/agent-exploration/scripts/dispatch-slices.sh is excluded by !.agents/**
• .agents/skills/agent-exploration/scripts/install-explorer.sh is excluded by !.agents/**
• .agents/skills/handoff/SKILL.md is excluded by !**/*.md, !.agents/**
• internal/sandbox/daytona/sidecar_assets/agh-daytona-sidecar-linux-amd64.gz is excluded by !**/*.gz
• internal/sandbox/daytona/sidecar_assets/agh-daytona-sidecar-linux-arm64.gz is excluded by !**/*.gz
• openapi/agh.json is excluded by !**/*.json
• packages/site/content/runtime/core/agents/providers.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/configuration/config-toml.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/operations/troubleshooting.mdx is excluded by !**/*.mdx
• skills-lock.json is excluded by !**/*.json
• web/src/generated/agh-openapi.d.ts is excluded by !**/generated/**

📒 Files selected for processing (83)

• .claudeignore
• .goreleaser.release-footer.md.tmpl
• internal/acp/failure.go
• internal/acp/failure_test.go
• internal/acp/provider_failure.go
• internal/api/contract/bridges.go
• internal/api/contract/contract.go
• internal/api/contract/settings.go
• internal/api/core/bridge_diagnostics.go
• internal/api/core/bridge_diagnostics_test.go
• internal/api/core/bridges.go
• internal/api/core/conversions.go
• internal/api/core/provider_auth_status_test.go
• internal/api/core/skills_test.go
• internal/api/spec/settings_test.go
• internal/api/spec/spec.go
• internal/api/spec/spec_test.go
• internal/bridges/diagnostics.go
• internal/bridges/diagnostics_test.go
• internal/cli/client.go
• internal/cli/client_actionable_errors_test.go
• internal/cli/daemon.go
• internal/cli/mcp_auth.go
• internal/cli/mcp_auth_test.go
• internal/cli/provider.go
• internal/cli/provider_test.go
• internal/cli/root.go
• internal/config/dotenv.go
• internal/config/dotenv_permissions_test.go
• internal/config/hermetic_env_test.go
• internal/config/provider.go
• internal/config/provider_alias_test.go
• internal/config/provider_test.go
• internal/config/release_config_test.go
• internal/daemon/boot.go
• internal/daemon/settings.go
• internal/daemon/settings_test.go
• internal/diagnostics/redact.go
• internal/diagnostics/redact_test.go
• internal/extension/bridge_delivery_integration_test.go
• internal/extension/manager_test.go
• internal/extension/reference_integration_test.go
• internal/modelcatalog/sources.go
• internal/modelcatalog/sources_test.go
• internal/providerauth/native_cli.go
• internal/providerenv/env.go
• internal/scheduler/scheduler_integration_test.go
• internal/session/manager_prompt.go
• internal/session/provider_runtime.go
• internal/session/provider_runtime_test.go
• internal/settings/collections.go
• internal/settings/mcp_runtime_status_test.go
• internal/settings/models.go
• internal/settings/provider_auth_status_test.go
• internal/settings/sections.go
• internal/settings/service.go
• internal/settings/skill_diagnostics_test.go
• internal/skills/diagnostics.go
• internal/skills/registry.go
• internal/skills/registry_agent.go
• internal/skills/registry_diagnostics.go
• internal/skills/registry_diagnostics_test.go
• internal/skills/registry_snapshot.go
• internal/skills/registry_workspace_cache.go
• internal/skills/types.go
• internal/store/globaldb/global_db_task_claim_adversarial_test.go
• internal/testutil/e2e/runtime_harness.go
• internal/testutil/hermetic_env.go
• internal/testutil/hermetic_env_test.go
• internal/tools/dispatch_test.go
• internal/tools/result_limit.go
• internal/transcript/markers.go
• internal/transcript/redaction.go
• internal/transcript/transcript.go
• internal/transcript/transcript_test.go
• internal/transcript/ui_messages.go
• internal/vault/crypto.go
• internal/vault/service_test.go
• packages/site/lib/__tests__/public-install-contract.test.ts
• packages/site/public/install.sh
• packages/site/scripts/__tests__/generate-changelog-release.test.ts
• packages/site/scripts/generate-changelog-release.ts
• web/src/lib/__tests__/settings-api-contract.test.ts

[coderabbitai]

🧹 Nitpick comments (1)

internal/bridges/diagnostics.go (1)

76-76: ⚡ Quick win

Variable diagnostics shadows the imported package name.

The local variable diagnostics on line 76 shadows the imported diagnostics package from line 7. This can cause confusion and potential bugs if future modifications attempt to call diagnostics.Redact() within this function scope.

♻️ Suggested rename to avoid shadowing

 func BuildBridgeDiagnostics(input BridgeDiagnosticsInput) []BridgeDiagnostic {
 	instance := input.Instance.normalize()
-	diagnostics := make([]BridgeDiagnostic, 0, 5)
-	diagnostics = append(diagnostics, providerDiagnostics(instance, input)...)
-	diagnostics = append(diagnostics, missingTokenDiagnostics(instance, input)...)
-	diagnostics = append(diagnostics, destinationDiagnostics(instance, input)...)
+	results := make([]BridgeDiagnostic, 0, 5)
+	results = append(results, providerDiagnostics(instance, input)...)
+	results = append(results, missingTokenDiagnostics(instance, input)...)
+	results = append(results, destinationDiagnostics(instance, input)...)
 	if diagnostic, ok := permissionDiagnostic(instance, input); ok {
-		diagnostics = append(diagnostics, diagnostic)
+		results = append(results, diagnostic)
 	}
 	if diagnostic, ok := transientDeliveryDiagnostic(instance, input); ok {
-		diagnostics = append(diagnostics, diagnostic)
+		results = append(results, diagnostic)
 	}
-	return diagnostics
+	return results
 }

🤖 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/bridges/diagnostics.go` at line 76, The local variable diagnostics
in the function that initializes BridgeDiagnostic slices shadows the imported
diagnostics package; rename the local variable (e.g., bridgeDiagnostics or
diagList) wherever it's declared and referenced (the make([]BridgeDiagnostic,
...) line and subsequent uses) so the package symbol diagnostics remains
available (e.g., to call diagnostics.Redact()). Update all references inside the
function to the new variable name.

🤖 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.

Nitpick comments:
In `@internal/bridges/diagnostics.go`:
- Line 76: The local variable diagnostics in the function that initializes
BridgeDiagnostic slices shadows the imported diagnostics package; rename the
local variable (e.g., bridgeDiagnostics or diagList) wherever it's declared and
referenced (the make([]BridgeDiagnostic, ...) line and subsequent uses) so the
package symbol diagnostics remains available (e.g., to call
diagnostics.Redact()). Update all references inside the function to the new
variable name.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 135fb078-e6e4-47fe-8b21-a9fde48bd068

📥 Commits

Reviewing files that changed from the base of the PR and between cc5821e and 4a2cfa1.

📒 Files selected for processing (10)

• internal/api/core/bridge_diagnostics.go
• internal/api/core/bridge_diagnostics_test.go
• internal/api/core/bridges.go
• internal/api/core/provider_auth_status_test.go
• internal/bridges/diagnostics.go
• internal/bridges/diagnostics_test.go
• internal/daemon/settings.go
• internal/daemon/settings_test.go
• internal/settings/models.go
• internal/settings/provider_auth_status_test.go