Skip to content

[MM-67954] Browser agent ltbrowserapi fails on startup: missing config/config.json when deploying from GitHub release#980

Merged
agarciamontoro merged 11 commits intomasterfrom
MM-67954
Mar 20, 2026
Merged

[MM-67954] Browser agent ltbrowserapi fails on startup: missing config/config.json when deploying from GitHub release#980
agarciamontoro merged 11 commits intomasterfrom
MM-67954

Conversation

@M-ZubairAhmed
Copy link
Copy Markdown
Member

@M-ZubairAhmed M-ZubairAhmed commented Mar 19, 2026

Summary

When deploying browser agents from the GitHub release tarball, ltbrowserapi fails with ENOENT: no such file or directory, open 'config/config.json' and enters a restart loop. This happens because:

  • The release tarball only ships *.sample.json files, not config.json
  • The browser service hard-requires config.json at startup for BrowserLogSettings
  • Terraform only uploads config.json to the first server agent, not to browser agents

This PR fixes the issue by:

  • Moving BrowserLogSettings from config.json into browsercontroller.json, eliminating the browser service's dependency on config.json
  • Uploading browsercontroller.json from the local machine to browser agent instances during Terraform provisioning
  • Validating the browser controller config on the agent before creating a browser load-test agent

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-67954

Summary by CodeRabbit

  • New Features

    • Browser agents now read and validate a dedicated browser-controller config; agent creation returns clear HTTP 400 errors when the config is missing or invalid.
  • Documentation

    • Added BrowserController LogSettings documentation describing console/file toggles, levels and file path.
  • Configuration

    • Browser logging moved to a dedicated browser-controller config; sample and example configs updated and global browser-specific block removed.
  • Tests

    • Added and expanded tests for browser-agent config validation and standardized agent-type test setup.

…le logging and set console log level to debug
…ests

- Introduced a new `setupAgentType` function to streamline the setup of agent types in tests.
- Updated `TestIsBrowserAgentInstance` and `TestBrowserAgentConfigValidation` to utilize the new setup function.
- Enhanced the `createLoadAgentHandler` to validate the `browsercontroller.json` configuration before creating browser agents.
- Added logic in the Terraform agent configuration to upload the `browsercontroller.json` to browser agent instances, ensuring valid configurations are used during load tests.
@M-ZubairAhmed M-ZubairAhmed added the 2: Dev Review Requires review by a core committer label Mar 19, 2026
@M-ZubairAhmed M-ZubairAhmed marked this pull request as ready for review March 19, 2026 23:47
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 19, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3fa09a25-2573-4f70-b0a9-9a1540c65400

📥 Commits

Reviewing files that changed from the base of the PR and between 251b21c and 05d413d.

📒 Files selected for processing (1)
  • api/agent_test.go

📝 Walkthrough

Walkthrough

API and Terraform now read and validate a dedicated BrowserController config (./config/browsercontroller.json) for browser agents. Frontend modules and tests updated to new loader/accessor paths and LogSettings shape; backend removed legacy BrowserLogSettings from central Config.

Changes

Cohort / File(s) Summary
API agent creation & tests
api/agent.go, api/agent_test.go, api/agent_client_test.go, api/client_test.go, api/coordinator_client_test.go, api/coordinator_test.go
Browser-agent create handler reads ./config/browsercontroller.json via browsercontroller.ReadConfig and validates it; on read/validation error returns HTTP 400 with error payload. Tests add setupAgentType helper and browser-config validation tests.
BrowserController config types & loader
loadtest/control/browsercontroller/config.go
Add exported Config and BrowserLogSettings types and ReadConfig(configFilePath string) (*Config, error) to load/validate ./config/browsercontroller.json.
Terraform agent deployment
deployment/terraform/agent.go
When outputs include browser agents, read and marshal browsercontroller.json and add upload step to place it at /home/{{.Username}}/.../config/browsercontroller.json; errors returned on failure.
Frontend config schema & loader refactor
browser/src/config/loader.ts, browser/src/config/accessors.ts
Rename schema to BrowserControllerConfigJsonSchema; remove exported configJson; accessors read browserControllerConfigJson.LogSettings.*; getMattermostServerURL() hardcoded to http://localhost:8065.
Frontend import path updates
browser/src/app.ts, browser/src/routes/browser.ts, browser/src/routes/browser.test.ts, browser/src/services/browser_manager.ts, browser/src/smoke_simulations/index.ts, browser/src/e2e/post_and_scroll_scenario.spec.ts, browser/src/logger/index.ts, browser/src/logger/index.test.ts
Update import paths to new modules (../config/accessors.js, ../logger/index.js, ../config/loader.js) and adjust tests/mocks to match new layout.
Config samples & examples
config/browsercontroller.sample.json, config/config.sample.json, config/config.sample.toml, examples/config/*/json/config.json
Add LogSettings to browsercontroller sample; remove legacy BrowserLogSettings from main samples and example configs.
Backend config struct cleanup & tests
loadtest/config.go, loadtest/loadtest_test.go, loadtest/user/userentity/helper_test.go
Remove BrowserLogSettings type and field from central Config; update tests to remove browser-specific logging from main Config.
Documentation
docs/config/browsercontroller.md, docs/config/config.md
Document new LogSettings under BrowserController; remove BrowserLogSettings section from main config docs.
Tests - logger & routes
browser/src/logger/index.test.ts, browser/src/routes/browser.test.ts
Adjust mocks/imports to new accessor paths; remove mocked getMattermostServerURL default in route tests and change negative test to assert 400 when server_url is missing.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant API as "API Server\n(api/agent.go)"
    participant BC as "BrowserController\n(loadtest/control/browsercontroller)"
    participant Defaults as "defaults.Validate"
    participant Terraform as "Terraform\n(deployment/terraform/agent.go)"

    Client->>API: POST /loadagent/create (browser agent)
    API->>BC: ReadConfig("./config/browsercontroller.json")
    BC-->>API: config or error
    API->>Defaults: Validate(config)
    Defaults-->>API: valid or validation error
    alt config valid
        API-->>Client: 201 Created (agent)
        Note right of Terraform: Deployment flow
        Terraform->>BC: Read & marshal browsercontroller.json
        Terraform-->>Agent: Upload browsercontroller.json to agent path
    else read/validation failed
        API-->>Client: 400 Bad Request { "error": "..." }
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.71% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the primary change: moving BrowserLogSettings from config.json to browsercontroller.json and adding validation, which directly addresses the deployment failure when config.json is missing from the GitHub release tarball.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch MM-67954
📝 Coding Plan
  • Generate coding plan for human review comments

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.3)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions


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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Caution

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

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

144-151: ⚠️ Potential issue | 🔴 Critical

Missing return after NewControllerWrapper error.

When NewControllerWrapper fails (line 145), the handler writes an error response but does not return early. Execution continues to line 153, which calls loadtest.New with a nil controller, likely causing a panic or unexpected behaviour.

🐛 Proposed fix
 	newC, err := NewControllerWrapper(&ltConfig, ucConfig, 0, agentId, a.metrics, isBAInstance)
 	if err != nil {
 		writeAgentResponse(w, http.StatusBadRequest, &client.AgentResponse{
 			Id:      agentId,
 			Message: "load-test agent creation failed",
 			Error:   fmt.Sprintf("could not create agent: %s", err),
 		})
+		return
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/agent.go` around lines 144 - 151, The error handler after calling
NewControllerWrapper(&ltConfig, ucConfig, 0, agentId, a.metrics, isBAInstance)
writes an AgentResponse but fails to return, allowing execution to continue with
a nil controller; update the handler so that after writeAgentResponse(...) it
immediately returns from the enclosing function (thus preventing subsequent
calls like loadtest.New(...) from running with newC == nil), referencing
NewControllerWrapper, writeAgentResponse, and the newC variable to locate and
fix the flow control.
🧹 Nitpick comments (2)
docs/config/browsercontroller.md (1)

40-89: Consider adding default values for LogSettings fields.

The Go struct in browsercontroller/config.go defines default values for these fields (e.g., EnableConsole: true, ConsoleLevel: "debug", FileLocation: "browseragent.log"), but they're not documented here. Adding Default: annotations would improve consistency with the other documented fields and help users understand the out-of-the-box behaviour.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/config/browsercontroller.md` around lines 40 - 89, The docs for
LogSettings are missing the default values present in the Go struct; update the
LogSettings section to add "Default:" annotations for each field to match the
defaults defined in the browsercontroller config (refer to the LogSettings
struct and fields in browsercontroller/config.go): set EnableConsole default to
true, ConsoleLevel default to "debug", EnableFile default to false (or whatever
the struct uses), FileLevel default to "debug" (or the struct value), and
FileLocation default to "browseragent.log"; ensure each field paragraph adds a
short "Default: <value>" line consistent with other docs.
browser/src/config/accessors.ts (1)

6-10: Tighten the docblock wording and typos for maintainability.

The comment has minor spelling/grammar issues (URl, its created) that make intent harder to scan.

Suggested wording update
 /**
- * Server URl is always passed as a parameter to the browser controller while
- * its created. So we don't need to read it from the config.json. But we need
- * hardcoded value for tests and smoke simulations.
+ * Server URL is always passed as a parameter when the browser controller is
+ * created, so we don't read it from config.json. We keep a hardcoded fallback
+ * for tests and smoke simulations.
  */
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@browser/src/config/accessors.ts` around lines 6 - 10, Edit the docblock
comment at the top of browser/src/config/accessors.ts to fix typos and tighten
wording: change "URl" to "URL", "its created" to "when it's created", and
rephrase the whole comment to a concise sentence like "The server URL is passed
to the browser controller when it's created, so we don't read it from
config.json; a hardcoded value is retained for tests and smoke simulations."
Ensure the updated comment replaces the existing block above the accessor
definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@api/agent_test.go`:
- Around line 400-418: The subtest "succeeds with valid browsercontroller.json"
depends on changing dirs and a repo-local config; instead create a temporary
config directory and a valid browsercontroller.json file inside it before
calling the POST("/create") path: make a temp dir (or use t.TempDir()), mkdir
"config" inside it, write a minimal valid JSON for browsercontroller.json, set
the test's current working dir to that temp dir (or ensure ReadConfig is pointed
there) and register t.Cleanup to remove the temp files and restore the original
working dir; remove the os.Chdir("..") reliance and keep the rest of the test
flow that uses requestData and the POST("/create") call unchanged so the test
becomes self-contained.

In `@deployment/terraform/agent.go`:
- Around line 230-237: The current upload branch only runs when agentType ==
deployment.AgentTypeBrowser, so browsercontroller.json isn't copied to hosts
where ltbrowserapi.service is present but agentType differs; change the
condition around creating the uploadInfo (the block that appends with srcData:
browserControllerConfig and dstPath via t.ExpandWithUser) to target any host
that will run ltbrowserapi.service instead of only deployment.AgentTypeBrowser —
e.g., detect presence of ltbrowserapi (a helper like
agentHasService("ltbrowserapi.service") or include other relevant AgentType
values) and append the uploadInfo accordingly so the file exists wherever the
code later restarts ltbrowserapi.service.
- Around line 112-115: The code currently hard-fails when
browsercontroller.ReadConfig("./config/browsercontroller.json") returns an
error; update the block guarded by t.output.HasBrowserAgents() so that if
reading "./config/browsercontroller.json" yields ENOENT (or can’t be found) you
fall back to reading a sample file (e.g.
"./config/browsercontroller.sample.json") and, if that also fails, fall back to
a built-in/default config (or browsercontroller.DefaultConfig()) instead of
returning an error; adjust the error handling around
browsercontroller.ReadConfig and the variable bccfg, log a warning when falling
back, and only return an error if no config can be established after the
fallbacks.

In `@docs/config/browsercontroller.md`:
- Line 55: Update the phrasing in the sentence "Possible values (in order of
decreasing verbosity, these are case sensitive):" and the other identical
occurrence to use the hyphenated compound adjective "case-sensitive" instead of
"case sensitive" so it reads "Possible values (in order of decreasing verbosity,
these are case-sensitive):".

---

Outside diff comments:
In `@api/agent.go`:
- Around line 144-151: The error handler after calling
NewControllerWrapper(&ltConfig, ucConfig, 0, agentId, a.metrics, isBAInstance)
writes an AgentResponse but fails to return, allowing execution to continue with
a nil controller; update the handler so that after writeAgentResponse(...) it
immediately returns from the enclosing function (thus preventing subsequent
calls like loadtest.New(...) from running with newC == nil), referencing
NewControllerWrapper, writeAgentResponse, and the newC variable to locate and
fix the flow control.

---

Nitpick comments:
In `@browser/src/config/accessors.ts`:
- Around line 6-10: Edit the docblock comment at the top of
browser/src/config/accessors.ts to fix typos and tighten wording: change "URl"
to "URL", "its created" to "when it's created", and rephrase the whole comment
to a concise sentence like "The server URL is passed to the browser controller
when it's created, so we don't read it from config.json; a hardcoded value is
retained for tests and smoke simulations." Ensure the updated comment replaces
the existing block above the accessor definitions.

In `@docs/config/browsercontroller.md`:
- Around line 40-89: The docs for LogSettings are missing the default values
present in the Go struct; update the LogSettings section to add "Default:"
annotations for each field to match the defaults defined in the
browsercontroller config (refer to the LogSettings struct and fields in
browsercontroller/config.go): set EnableConsole default to true, ConsoleLevel
default to "debug", EnableFile default to false (or whatever the struct uses),
FileLevel default to "debug" (or the struct value), and FileLocation default to
"browseragent.log"; ensure each field paragraph adds a short "Default: <value>"
line consistent with other docs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1ed82073-e855-452f-b440-0bdca7d3c0f7

📥 Commits

Reviewing files that changed from the base of the PR and between 43de7d6 and ab40453.

📒 Files selected for processing (24)
  • api/agent.go
  • api/agent_test.go
  • browser/src/app.ts
  • browser/src/config/accessors.ts
  • browser/src/config/loader.ts
  • browser/src/e2e/post_and_scroll_scenario.spec.ts
  • browser/src/logger/index.test.ts
  • browser/src/logger/index.ts
  • browser/src/routes/browser.test.ts
  • browser/src/routes/browser.ts
  • browser/src/services/browser_manager.ts
  • browser/src/smoke_simulations/index.ts
  • config/browsercontroller.sample.json
  • config/config.sample.json
  • config/config.sample.toml
  • deployment/terraform/agent.go
  • docs/config/browsercontroller.md
  • docs/config/config.md
  • examples/config/perfcomp/json/config.json
  • examples/config/release/json/config.json
  • loadtest/config.go
  • loadtest/control/browsercontroller/config.go
  • loadtest/loadtest_test.go
  • loadtest/user/userentity/helper_test.go
💤 Files with no reviewable changes (7)
  • config/config.sample.toml
  • examples/config/perfcomp/json/config.json
  • loadtest/loadtest_test.go
  • examples/config/release/json/config.json
  • config/config.sample.json
  • docs/config/config.md
  • loadtest/config.go

Copy link
Copy Markdown
Member

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

Looking good! This makes sense, sending the file to the agent machine on creation seems robust to me. I added a couple of comments, and I'll take the liberty to fix the failing CI test so that we can try and merge this today.

Comment on lines +125 to +142
// Read and validate the browsercontroller.json that was uploaded by
// Terraform to confirm it landed correctly and contains valid values
// before proceeding with browser agent creation if it's a browser agent instance.
if isBAInstance {
bccfg, err := browsercontroller.ReadConfig("./config/browsercontroller.json")
if err != nil {
writeAgentResponse(w, http.StatusBadRequest, &client.AgentResponse{
Error: fmt.Sprintf("could not read browser controller config: %s", err),
})
return
}
if err := defaults.Validate(bccfg); err != nil {
writeAgentResponse(w, http.StatusBadRequest, &client.AgentResponse{
Error: fmt.Sprintf("could not validate browser controller config: %s", err),
})
return
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just to confirm for my own understanding: this is only to prevent malformed or missing config files, right? The ltbrowserapi process is the one that will read this later.

@agarciamontoro
Copy link
Copy Markdown
Member

@CodeRabbit review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 20, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
api/agent_test.go (1)

35-47: Consider using t.Setenv for cleaner environment variable handling.

Go 1.17+ provides t.Setenv which automatically restores the original value at test cleanup and marks the test as incompatible with t.Parallel(). This would simplify the helper and make the non-parallel requirement explicit.

♻️ Suggested refactor
 func setupAgentType(t *testing.T, agentType string) {
 	t.Helper()
-	originalHome := os.Getenv("HOME")
 	tempDir, err := os.MkdirTemp("", "test_home_"+agentType)
 	require.NoError(t, err)
+	t.Cleanup(func() { os.RemoveAll(tempDir) })
+
 	agentTypeFile := filepath.Join(tempDir, deployment.AgentTypeFileName)
 	require.NoError(t, os.WriteFile(agentTypeFile, []byte(agentType), 0644))
-	os.Setenv("HOME", tempDir)
-	t.Cleanup(func() {
-		os.Setenv("HOME", originalHome)
-		os.RemoveAll(tempDir)
-	})
+	t.Setenv("HOME", tempDir)
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/agent_test.go` around lines 35 - 47, The setupAgentType helper should use
t.Setenv to set HOME and let the testing framework restore it automatically:
create the tempDir and agentType file as before, call t.Setenv("HOME", tempDir)
instead of os.Setenv and remove the manual originalHome capture and os.Setenv
restore; keep a t.Cleanup to os.RemoveAll(tempDir) to delete the temp directory;
update the function setupAgentType to use t.Setenv and remove the manual
environment restore logic so tests are marked as incompatible with t.Parallel().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@api/agent_test.go`:
- Around line 35-47: The setupAgentType helper should use t.Setenv to set HOME
and let the testing framework restore it automatically: create the tempDir and
agentType file as before, call t.Setenv("HOME", tempDir) instead of os.Setenv
and remove the manual originalHome capture and os.Setenv restore; keep a
t.Cleanup to os.RemoveAll(tempDir) to delete the temp directory; update the
function setupAgentType to use t.Setenv and remove the manual environment
restore logic so tests are marked as incompatible with t.Parallel().

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b024b2c0-3cec-4b8b-aa96-919cd2e29b4c

📥 Commits

Reviewing files that changed from the base of the PR and between ca6e036 and 11aec15.

📒 Files selected for processing (3)
  • api/agent_test.go
  • browser/src/config/accessors.ts
  • docs/config/browsercontroller.md
✅ Files skipped from review due to trivial changes (1)
  • docs/config/browsercontroller.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • browser/src/config/accessors.ts

@agarciamontoro
Copy link
Copy Markdown
Member

Go 1.17+ provides t.Setenv which automatically restores the original value at test cleanup and marks the test as incompatible with t.Parallel(). This would simplify the helper and make the non-parallel requirement explicit.

Ah, this is nice!

This takes care of cleanup automatically and marks the test as
not suitable for test parallelism.
@agarciamontoro
Copy link
Copy Markdown
Member

@coderabbitai configuration

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
api/agent_test.go (1)

380-387: Make the “missing config” subtest fully self-contained.

This case still depends on the process working directory not containing ./config/browsercontroller.json. Consider forcing an empty temp working dir in this subtest too, so it can’t pass/fail based on ambient files.

Suggested patch
 t.Run("fails when browsercontroller.json is missing", func(t *testing.T) {
 	setupAgentType(t, deployment.AgentTypeBrowser)
+	tempDir := t.TempDir()
+	t.Chdir(tempDir)

 	rd := requestData{LoadTestConfig: ltConfig}
 	e.POST("/create").WithQuery("id", "ltb0").WithJSON(rd).
 		Expect().Status(http.StatusBadRequest).
 		JSON().Object().ContainsKey("error")
 })
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/agent_test.go` around lines 380 - 387, The subtest started with
t.Run("fails when browsercontroller.json is missing", ...) is not fully
self-contained because it relies on the process working directory not containing
./config/browsercontroller.json; wrap the test body so it creates an isolated
working directory via tmp := t.TempDir(), save the current working dir (cwd, err
:= os.Getwd()), chdir into tmp (os.Chdir(tmp)) and defer restoring the original
cwd (defer func(){ _ = os.Chdir(cwd) }()), then call setupAgentType(t,
deployment.AgentTypeBrowser) and run the existing requestData/ e.POST("/create")
assertions; this guarantees the missing browsercontroller.json condition
regardless of ambient files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@api/agent_test.go`:
- Around line 380-387: The subtest started with t.Run("fails when
browsercontroller.json is missing", ...) is not fully self-contained because it
relies on the process working directory not containing
./config/browsercontroller.json; wrap the test body so it creates an isolated
working directory via tmp := t.TempDir(), save the current working dir (cwd, err
:= os.Getwd()), chdir into tmp (os.Chdir(tmp)) and defer restoring the original
cwd (defer func(){ _ = os.Chdir(cwd) }()), then call setupAgentType(t,
deployment.AgentTypeBrowser) and run the existing requestData/ e.POST("/create")
assertions; this guarantees the missing browsercontroller.json condition
regardless of ambient files.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3b9015f5-4bed-47ea-af4d-2a53ec01f1c8

📥 Commits

Reviewing files that changed from the base of the PR and between 11aec15 and 251b21c.

📒 Files selected for processing (1)
  • api/agent_test.go

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 20, 2026

Configuration used: Organization UI

CodeRabbit configuration
language: en-CA
tone_instructions: ''
early_access: false
enable_free_tier: true
inheritance: false
reviews:
  profile: chill
  request_changes_workflow: true
  high_level_summary: true
  high_level_summary_instructions: ''
  high_level_summary_placeholder: '@coderabbitai summary'
  high_level_summary_in_walkthrough: false
  auto_title_placeholder: '@coderabbitai'
  auto_title_instructions: ''
  review_status: false
  review_details: false
  commit_status: true
  fail_commit_status: false
  collapse_walkthrough: true
  changed_files_summary: true
  sequence_diagrams: true
  estimate_code_review_effort: true
  assess_linked_issues: true
  related_issues: true
  related_prs: true
  suggested_labels: true
  labeling_instructions: []
  auto_apply_labels: false
  suggested_reviewers: false
  auto_assign_reviewers: false
  in_progress_fortune: false
  poem: false
  enable_prompt_for_ai_agents: true
  path_filters: []
  path_instructions: []
  abort_on_close: true
  disable_cache: false
  auto_review:
    enabled: true
    description_keyword: ''
    auto_incremental_review: true
    auto_pause_after_reviewed_commits: 5
    ignore_title_keywords: []
    labels: []
    drafts: false
    base_branches: []
    ignore_usernames: []
  finishing_touches:
    docstrings:
      enabled: true
    unit_tests:
      enabled: true
    simplify:
      enabled: false
    custom: []
  pre_merge_checks:
    override_requested_reviewers_only: false
    docstrings:
      mode: warning
      threshold: 80
    title:
      mode: warning
      requirements: ''
    description:
      mode: warning
    issue_assessment:
      mode: warning
    custom_checks: []
  tools:
    ast-grep:
      rule_dirs: []
      util_dirs: []
      essential_rules: true
      packages: []
    shellcheck:
      enabled: true
    ruff:
      enabled: true
    markdownlint:
      enabled: true
    github-checks:
      enabled: true
      timeout_ms: 90000
    languagetool:
      enabled: true
      enabled_rules: []
      disabled_rules: []
      enabled_categories: []
      disabled_categories: []
      enabled_only: false
      level: default
    biome:
      enabled: true
    hadolint:
      enabled: true
    swiftlint:
      enabled: true
    phpstan:
      enabled: true
      level: default
    phpmd:
      enabled: true
    phpcs:
      enabled: true
    golangci-lint:
      enabled: true
    yamllint:
      enabled: true
    gitleaks:
      enabled: true
    trufflehog:
      enabled: true
    checkov:
      enabled: true
    tflint:
      enabled: true
    detekt:
      enabled: true
    eslint:
      enabled: true
    flake8:
      enabled: true
    fortitudeLint:
      enabled: true
    rubocop:
      enabled: true
    buf:
      enabled: true
    regal:
      enabled: true
    actionlint:
      enabled: true
    pmd:
      enabled: true
    clang:
      enabled: true
    cppcheck:
      enabled: true
    opengrep:
      enabled: true
    semgrep:
      enabled: true
    circleci:
      enabled: true
    clippy:
      enabled: true
    sqlfluff:
      enabled: true
    trivy:
      enabled: true
    prismaLint:
      enabled: true
    pylint:
      enabled: true
    oxc:
      enabled: true
    shopifyThemeCheck:
      enabled: true
    luacheck:
      enabled: true
    brakeman:
      enabled: true
    dotenvLint:
      enabled: true
    htmlhint:
      enabled: true
    stylelint:
      enabled: true
    checkmake:
      enabled: true
    osvScanner:
      enabled: true
    blinter:
      enabled: true
    smartyLint:
      enabled: true
    emberTemplateLint:
      enabled: true
    psscriptanalyzer:
      enabled: true
chat:
  art: false
  allow_non_org_members: true
  auto_reply: true
  integrations:
    jira:
      usage: auto
    linear:
      usage: auto
knowledge_base:
  opt_out: false
  web_search:
    enabled: true
  code_guidelines:
    enabled: true
    filePatterns: []
  learnings:
    scope: auto
  issues:
    scope: auto
  jira:
    usage: auto
    project_keys: []
  linear:
    usage: auto
    team_keys: []
  pull_requests:
    scope: auto
  mcp:
    usage: auto
    disabled_servers: []
  linked_repositories: []
code_generation:
  docstrings:
    language: en-US
    path_instructions: []
  unit_tests:
    path_instructions: []
issue_enrichment:
  auto_enrich:
    enabled: false
  planning:
    enabled: true
    auto_planning:
      enabled: true
      labels: []
  labeling:
    labeling_instructions: []
    auto_apply_labels: false

The testing version takes care of cleanup automatically.
@agarciamontoro agarciamontoro self-requested a review March 20, 2026 13:06
@agarciamontoro agarciamontoro removed the request for review from fmartingr March 20, 2026 13:07
@agarciamontoro
Copy link
Copy Markdown
Member

Removing @fmartingr following the new 1 human reviewer rule :) I'll merge this as soon as I test it.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 20, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@agarciamontoro
Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 20, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@agarciamontoro
Copy link
Copy Markdown
Member

@coderabbitai resolve

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 20, 2026

✅ Actions performed

Comments resolved and changes approved.

@agarciamontoro
Copy link
Copy Markdown
Member

Ok, that was difficult.

@agarciamontoro agarciamontoro merged commit 01bcaa2 into master Mar 20, 2026
1 check passed
@agarciamontoro agarciamontoro deleted the MM-67954 branch March 20, 2026 15:15
agarciamontoro added a commit that referenced this pull request Mar 20, 2026
…g/config.json when deploying from GitHub release (#980)

* Moved configuration accessors to a new file structure under `config/`

* Refactor logger tests to use updated config accessors path and mock node modules correctly

* Moved LogSettings in browsercontroller.json to manage console and file logging from config.json

* Update default logging settings in BrowserLogSettings to enable console logging and set console log level to debug

* Add browser controller configuration validation and setup for agent tests

- Introduced a new `setupAgentType` function to streamline the setup of agent types in tests.
- Updated `TestIsBrowserAgentInstance` and `TestBrowserAgentConfigValidation` to utilize the new setup function.
- Enhanced the `createLoadAgentHandler` to validate the `browsercontroller.json` configuration before creating browser agents.
- Added logic in the Terraform agent configuration to upload the `browsercontroller.json` to browser agent instances, ensuring valid configurations are used during load tests.

* fix test

* Fix test with valid config file

* Fix typo in comment

* Fix typo in docs

* Use t.Chdir and t.Setenv instead of os's

This takes care of cleanup automatically and marks the test as
not suitable for test parallelism.

* Use t.TempDir instead of os.MkdirTemp

The testing version takes care of cleanup automatically.

---------

Co-authored-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>
@agarciamontoro
Copy link
Copy Markdown
Member

/cherry-pick release-1.33

@mattermost-build
Copy link
Copy Markdown

Cherry pick is scheduled.

@mattermost-build
Copy link
Copy Markdown

Error trying doing the automated Cherry picking. Please do this manually


This was referenced Mar 20, 2026
agarciamontoro added a commit that referenced this pull request Mar 22, 2026
…g/config.json when deploying from GitHub release (#980) (#981)

* Moved configuration accessors to a new file structure under `config/`

* Refactor logger tests to use updated config accessors path and mock node modules correctly

* Moved LogSettings in browsercontroller.json to manage console and file logging from config.json

* Update default logging settings in BrowserLogSettings to enable console logging and set console log level to debug

* Add browser controller configuration validation and setup for agent tests

- Introduced a new `setupAgentType` function to streamline the setup of agent types in tests.
- Updated `TestIsBrowserAgentInstance` and `TestBrowserAgentConfigValidation` to utilize the new setup function.
- Enhanced the `createLoadAgentHandler` to validate the `browsercontroller.json` configuration before creating browser agents.
- Added logic in the Terraform agent configuration to upload the `browsercontroller.json` to browser agent instances, ensuring valid configurations are used during load tests.

* fix test

* Fix test with valid config file

* Fix typo in comment

* Fix typo in docs

* Use t.Chdir and t.Setenv instead of os's

This takes care of cleanup automatically and marks the test as
not suitable for test parallelism.

* Use t.TempDir instead of os.MkdirTemp

The testing version takes care of cleanup automatically.

---------

Co-authored-by: M-ZubairAhmed <m-zubairahmed@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2: Dev Review Requires review by a core committer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants