test: improve coverage

[omarluq]

No description provided.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 842a2d19-3092-4d35-813f-a8b914b9c166

📥 Commits

Reviewing files that changed from the base of the PR and between 9a50df3 and 8a514b7.

📒 Files selected for processing (5)

• internal/assistant/runtime_slash_internal_test.go
• internal/provider/client_complete_internal_test.go
• internal/provider/http.go
• internal/provider/http_internal_test.go
• internal/provider/test_helpers_internal_test.go

🚧 Files skipped from review as they are similar to previous changes (3)

• internal/provider/test_helpers_internal_test.go
• internal/provider/client_complete_internal_test.go
• internal/assistant/runtime_slash_internal_test.go

---

📝 Walkthrough

Summary by CodeRabbit

• Tests
  • Expanded test coverage across skill handling, lifecycle/tool mutation behavior, Lua schema/value conversions, and tool validation/argument parsing edge cases.
  • Added new scenarios for “no skills” responses, limit-aware skill reads, context cancellation behavior, and stronger Lua parsing acceptance/rejection rules.
  • Improved schema/JSON robustness checks (including empty-schema handling, invalid stored raw fallbacks, and error-message consistency).
  • Enhanced coverage for tool execution errors, numeric argument rendering, and provider completion dispatch routing across API variants.

Walkthrough

This PR adds and refactors tests across five internal packages (tool, extension, provider, terminal, assistant). Coverage is expanded for previously untested branches in tool argument/schema/validation, Lua value and schema encoding, extension lifecycle dispatch, provider API routing, terminal display helpers, and assistant skill command execution. Provider HTTP header constants are extracted from inline literals into a shared const block.

Changes

Test Coverage Expansion and Constant Extraction

Layer / File(s)	Summary
Tool schema, arguments, validation, and registry internal/tool/arguments_internal_test.go, internal/tool/schema_test.go, internal/tool/registry_test.go, internal/tool/input_validation_internal_test.go	Adds TestArgumentsInvalidStoredRawFallbacks for corrupt raw data; TestSchemaFromRawCopiesInput for copy-on-create semantics; TestRegistry_ExecuteErrors for unknown-tool and invalid-JSON paths; introduces shared error constants; replaces single-case schema rejection test with table-driven TestValidateToolInputSchemaErrors; adds compile-cache error, normalization fallback, and required-argument edge-case tests.
Extension Lua values and table handling internal/extension/lua_values_internal_test.go	Adds TestJSONRawToLuaValue for JSON-to-Lua conversion; TestNewLuaValueCoversCollectionAndFallbackTypes for map/slice-to-table conversion; TestLuaValueFallbackBranches for missing-key and missing-function defaults; TestLuaUIDrawOpReadsSpans for nested Lua table span reading; adjusts luaFieldWorkingForTest constant usage.
Extension Lua schema encoding and errors internal/extension/lua_schema_internal_test.go	Adds math import; adds three tests: TestLuaSchemaRawEncodesArraySeparatorsAndMixedKeys for array vs. object distinction, TestLuaSchemaRawReturnsFirstNestedArrayError for non-finite error in nested arrays, TestLuaSchemaObjectStopsAfterFirstEncodeError for early error termination.
Extension tool-call mutations and lifecycle dispatch internal/extension/lifecycle_internal_test.go, internal/extension/manager_test.go	Adds TestMergeToolCallMutationBranches, TestToolCallMutationFromLuaFallbacks, TestToolCallMutationFromLuaAcceptsExplicitEmptyArguments for mutation merging and Lua parsing; adds TestLifecycleMutationHelpersFallbacks, TestDispatchLifecycleReturnsContextErrorBeforeHandler for dispatch fallbacks; adds toolCallMutationTable helper; converts manager invalid-schema test to table-driven.
Provider test constants and Codex header constants internal/provider/test_helpers_internal_test.go, internal/provider/http.go, internal/provider/http_internal_test.go	Introduces testProviderAPIKey, testProviderAccountID, testProviderBaseURL shared test constants; extracts Codex header names and values into const block in http.go; updates all Codex header tests to use the new constants.
Provider completion dispatch and schema tests internal/provider/client_complete_internal_test.go, internal/provider/tool_schema_internal_test.go	Updates existing tests to use shared test constants; adds TestHTTPCompletionClientCompleteDispatchesProviderAPIs covering OpenAI, OpenAI-codex, and Anthropic API routing; introduces testProviderCapturedRequest and testProviderHTTPClientWithCapture for request capture; adds TestToolParameterSchemaMarshalJSONEmptySchema.
Terminal tool display and constants internal/terminal/test_constants_internal_test.go, internal/terminal/running_tools_internal_test.go, internal/terminal/tool_display_internal_test.go	Adds testToolCommandKey constant; updates tests to use testutil.ToolArguments with the constant; adds TestToolDisplayAdditionalFallbackBranches and TestToolSummaryNumericArgumentTypes covering fallback paths and numeric timeout formatting; adds bashSummaryForTest helper.
Assistant skill command and skill-read execution internal/assistant/runtime_slash_internal_test.go	Adds runtimeSlashSkill helper for consistent test skill creation; updates TestRespondToSkillCommand to set HOME via t.TempDir; adds TestRespondToSkillCommandWithNoSkills for empty-skills path; refactors TestLoadSkillWithReadToolReportsReadErrors to use helper; adds TestLoadSkillWithReadToolUsesLimit for limit parameter behavior; updates TestSkillReadArgumentsJSON to use local limit variable.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• omarluq/librecode#48: Introduces the tool-call/tool-result lifecycle hook and mutation support that the new TestMergeToolCallMutationBranches and TestToolCallMutationFromLua* tests directly exercise.
• omarluq/librecode#63: Introduces the slash/skill runtime logic exercised by the updated TestRespondToSkillCommand and new TestLoadSkillWithReadToolUsesLimit tests in internal/assistant/runtime_slash_internal_test.go.
• omarluq/librecode#137: Refactors running-tools and tool-display test cases; the main PR updates the same test files to use testutil.ToolArguments with testToolCommandKey.

Poem

🐇 Hop, hop, through the test suite I go,
Checking each branch where the edge cases flow,
No skills found? I assert it with glee!
Non-finite numbers? Errors for free!
Headers extracted to constants so bright,
The rabbit tests on from morning to night! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (2 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'test: improve coverage' is vague and generic, using non-descriptive terms that don't convey specific information about the actual changes.	Consider using a more specific title that describes the primary testing areas, such as 'test: add coverage for lifecycle, schema, and tool validation' or similar.
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to assess relevance to the changeset.	Add a pull request description that outlines the main testing improvements and why they were added.

✅ Passed checks (2 passed)

Check name	Status	Explanation
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.

_{✏️ 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 test/improve-coverage

---

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.40%. Comparing base (4e35dea) to head (8a514b7).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
+ Coverage   81.88%   82.40%   +0.51%     
==========================================
  Files         284      284              
  Lines       22128    22128              
==========================================
+ Hits        18120    18234     +114     
+ Misses       2827     2754      -73     
+ Partials     1181     1140      -41     

Flag	Coverage Δ
unittests	82.40% <100.00%> (+0.51%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

internal/provider/client_complete_internal_test.go (1)

93-133: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Strengthen dispatch assertions to verify route selection, not only output.

On Line 93, this test proves provider parsers can return "ok", but it doesn’t assert which endpoint/headers were used per API case. A routing regression between OpenAI responses and Codex could still pass with identical stubbed bodies. Capture request path (and Codex-specific headers) in the test HTTP stub and assert per case.

🤖 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/provider/client_complete_internal_test.go` around lines 93 - 133,
The test TestHTTPCompletionClientCompleteDispatchesProviderAPIs only verifies
that response parsing returns the expected output text, but does not verify that
the correct endpoint or headers were used for each API type. Enhance the
testProviderHTTPClient stub to capture the actual HTTP request details (path and
headers) instead of just returning a predefined response, then add assertions
within each test case iteration to verify that the correct endpoint path and
API-specific headers (particularly Codex-specific headers for the
apiOpenAICodexResponses case) were used when client.Complete was called. This
ensures routing regressions between the different API providers cannot be masked
by identical stubbed response bodies.

🤖 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/assistant/runtime_slash_internal_test.go`:
- Line 187: The assert.JSONEq call on line 187 directly interpolates the
skillPath variable into a JSON literal string, which fails on Windows because
backslashes in file paths are not properly escaped as JSON. Instead of string
interpolation, construct the expected payload using a struct or map and marshal
it to JSON using json.Marshal, which will properly escape all special characters
including backslashes, ensuring the assertion works correctly across all
platforms.

---

Nitpick comments:
In `@internal/provider/client_complete_internal_test.go`:
- Around line 93-133: The test
TestHTTPCompletionClientCompleteDispatchesProviderAPIs only verifies that
response parsing returns the expected output text, but does not verify that the
correct endpoint or headers were used for each API type. Enhance the
testProviderHTTPClient stub to capture the actual HTTP request details (path and
headers) instead of just returning a predefined response, then add assertions
within each test case iteration to verify that the correct endpoint path and
API-specific headers (particularly Codex-specific headers for the
apiOpenAICodexResponses case) were used when client.Complete was called. This
ensures routing regressions between the different API providers cannot be masked
by identical stubbed response bodies.

🪄 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 Plus

Run ID: 5d4b236a-bdc5-4e72-a17f-77c02890baf5

📥 Commits

Reviewing files that changed from the base of the PR and between 4e35dea and 9a50df3.

📒 Files selected for processing (15)

• internal/assistant/runtime_slash_internal_test.go
• internal/extension/lifecycle_internal_test.go
• internal/extension/lua_schema_internal_test.go
• internal/extension/lua_values_internal_test.go
• internal/extension/manager_test.go
• internal/provider/client_complete_internal_test.go
• internal/provider/test_helpers_internal_test.go
• internal/provider/tool_schema_internal_test.go
• internal/terminal/running_tools_internal_test.go
• internal/terminal/test_constants_internal_test.go
• internal/terminal/tool_display_internal_test.go
• internal/tool/arguments_internal_test.go
• internal/tool/input_validation_internal_test.go
• internal/tool/registry_test.go
• internal/tool/schema_test.go

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud