test: increase test coverage in pkg/flags, pkg/filesystem, pkg/http, and pkg/function#2173
Open
test: increase test coverage in pkg/flags, pkg/filesystem, pkg/http, and pkg/function#2173
Conversation
mergify
bot
added
triage
…on packages Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
nitrocode
added
the
no-release
Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Expand tests in area with lowest coverage
test: increase test coverage in pkg/flags, pkg/filesystem, pkg/http, and pkg/function
Mar 12, 2026
mergify
bot
removed
the
wip
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2173 +/- ##
==========================================
+ Coverage 77.23% 77.56% +0.33%
==========================================
Files 1020 1021 +1
Lines 96481 96589 +108
==========================================
+ Hits 74515 74918 +403
+ Misses 17766 17481 -285
+ Partials 4200 4190 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Member
|
@copilot fix all the failures |
Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
osterman
previously approved these changes
Mar 15, 2026
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Copilot stopped work on behalf of
nitrocode due to an error
March 24, 2026 02:54
…t ports in normalizeHost Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com> Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/a2999beb-54fd-42e7-b1f4-c50844590385
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/http/client.go (2)
257-261:⚠️ Potential issue | 🟡 MinorKeep
User-Agentinside the auth-injection branch.The transport still rewrites
User-Agenteven when it intentionally preserves a caller-suppliedAuthorization. Move that write under the innerAuthorization == ""check so caller-managed requests stay untouched.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/http/client.go` around lines 257 - 261, The transport currently sets the "User-Agent" header whenever scheme=="https" && matcher(host) && t.GitHubToken != "", even for requests where the caller already provided an Authorization header; move the reqClone.Header.Set("User-Agent", userAgent) into the inner if that checks reqClone.Header.Get("Authorization") == "" so that User-Agent is only injected when the transport also injects Authorization (i.e., inside the same branch that sets Authorization), leaving caller-supplied Authorization and User-Agent untouched; update the block around variables scheme, matcher(host), t.GitHubToken and reqClone.Header accordingly.
97-103:⚠️ Potential issue | 🟡 MinorCanonicalize redirect origins before stripping auth.
This guard still compares raw
Hoststrings. Case-only, trailing-dot, and default-port variants of the same origin can be treated as cross-host here, which can drop a caller-suppliedAuthorizationon a same-origin redirect and let the wrapper replace it on the next hop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/http/client.go` around lines 97 - 103, stripAuthOnCrossHostRedirect currently compares raw req.URL.Host vs via[0].URL.Host which can differ by case, trailing dots, or implicit default ports and incorrectly drop Authorization; update the function to canonicalize both endpoints before comparing by using URL.Hostname() lowercased (trim any trailing dot) and resolve port via URL.Port() with the scheme's default (80 for http, 443 for https) so you compare normalized host + port pairs, and only call req.Header.Del("Authorization") when those normalized origins differ; ensure the function still enforces the redirect-limit error and returns nil on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 14-18: Update the CHANGELOG text to accurately reflect the runtime
behavior implemented in applyGlobCacheConfig: state that only positive values
below the minimums are clamped up to the floor, whereas zero or negative values
for ATMOS_FS_GLOB_CACHE_MAX_ENTRIES, ATMOS_FS_GLOB_CACHE_TTL, and
ATMOS_FS_GLOB_CACHE_EMPTY cause the code to fall back to the default values
rather than being clamped; mention applyGlobCacheConfig by name so readers and
reviewers can find the exact logic.
In `@pkg/http/client.go`:
- Around line 98-99: Replace the ad-hoc errors.New("stopped after 10 redirects")
in the redirect handling branch (the code that checks len(via) >= 10) with a
static sentinel error from the repo error taxonomy: either reference the
existing sentinel in errors/errors.go (e.g., errutils.ErrTooManyRedirects) or
add a new exported sentinel like ErrRedirectLimitExceeded to errors/errors.go
and use that here; ensure you return the sentinel (not a new error string) so
the checkable static error from errors/errors.go is used throughout the
codebase.
---
Duplicate comments:
In `@pkg/http/client.go`:
- Around line 257-261: The transport currently sets the "User-Agent" header
whenever scheme=="https" && matcher(host) && t.GitHubToken != "", even for
requests where the caller already provided an Authorization header; move the
reqClone.Header.Set("User-Agent", userAgent) into the inner if that checks
reqClone.Header.Get("Authorization") == "" so that User-Agent is only injected
when the transport also injects Authorization (i.e., inside the same branch that
sets Authorization), leaving caller-supplied Authorization and User-Agent
untouched; update the block around variables scheme, matcher(host),
t.GitHubToken and reqClone.Header accordingly.
- Around line 97-103: stripAuthOnCrossHostRedirect currently compares raw
req.URL.Host vs via[0].URL.Host which can differ by case, trailing dots, or
implicit default ports and incorrectly drop Authorization; update the function
to canonicalize both endpoints before comparing by using URL.Hostname()
lowercased (trim any trailing dot) and resolve port via URL.Port() with the
scheme's default (80 for http, 443 for https) so you compare normalized host +
port pairs, and only call req.Header.Del("Authorization") when those normalized
origins differ; ensure the function still enforces the redirect-limit error and
returns nil on success.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 602ff525-3eaa-44e0-bd23-d013c3bef84b
📒 Files selected for processing (6)
CHANGELOG.mdpkg/filesystem/doc.gopkg/filesystem/glob.gopkg/filesystem/glob_atomic_test.gopkg/http/client.gopkg/http/client_test.go
✅ Files skipped from review due to trivial changes (3)
- pkg/filesystem/doc.go
- pkg/http/client_test.go
- pkg/filesystem/glob_atomic_test.go
Comment on lines
+14
to
+18
| The cache is now bounded and configurable via three environment variables: | ||
| - `ATMOS_FS_GLOB_CACHE_MAX_ENTRIES` (default `1024`, minimum `16`) — maximum number of cached glob patterns. | ||
| - `ATMOS_FS_GLOB_CACHE_TTL` (default `5m`, minimum `1s`) — time-to-live for each cache entry. | ||
| Values below the respective minimums are clamped up rather than rejected. | ||
| - `ATMOS_FS_GLOB_CACHE_EMPTY` (default `1`) — set to `0` to skip caching patterns that match no files. |
Contributor
There was a problem hiding this comment.
Clarify how non-positive cache settings behave.
applyGlobCacheConfig only clamps positive values below the floor. 0 and negative values currently fall back to defaults, so "values below the respective minimums are clamped up" reads broader than the shipped behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 14 - 18, Update the CHANGELOG text to accurately
reflect the runtime behavior implemented in applyGlobCacheConfig: state that
only positive values below the minimums are clamped up to the floor, whereas
zero or negative values for ATMOS_FS_GLOB_CACHE_MAX_ENTRIES,
ATMOS_FS_GLOB_CACHE_TTL, and ATMOS_FS_GLOB_CACHE_EMPTY cause the code to fall
back to the default values rather than being clamped; mention
applyGlobCacheConfig by name so readers and reviewers can find the exact logic.
pkg/http/client.go
Outdated
Comment on lines
+98
to
+99
| if len(via) >= 10 { | ||
| return errors.New("stopped after 10 redirects") |
Contributor
There was a problem hiding this comment.
Use a static sentinel for the redirect cap.
This new errors.New(...) path bypasses the repo’s error taxonomy. Please wrap an existing errUtils sentinel here, or add a dedicated redirect-limit sentinel in errors/errors.go. As per coding guidelines, "All errors MUST be wrapped using static errors defined in errors/errors.go."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/http/client.go` around lines 98 - 99, Replace the ad-hoc
errors.New("stopped after 10 redirects") in the redirect handling branch (the
code that checks len(via) >= 10) with a static sentinel error from the repo
error taxonomy: either reference the existing sentinel in errors/errors.go
(e.g., errutils.ErrTooManyRedirects) or add a new exported sentinel like
ErrRedirectLimitExceeded to errors/errors.go and use that here; ensure you
return the sentinel (not a new error string) so the checkable static error from
errors/errors.go is used throughout the codebase.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Request timed out after 1200000ms (requestId=e0c6c363-29bc-4b92-a8a2-dbeb271d7e1e) |
Fixed 3 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…e-open test - Add missing trailing newlines to CHANGELOG.md, errors/errors.go, and pkg/http/client.go (CodeRabbit autofix had incorrectly removed them in the previous commit, breaking pre-commit end-of-file-fixer hook) - Replace TestWriteFileAtomicWindows_OverwriteWhileOpenForRead with TestWriteFileAtomicWindows_ModePreserved: the original test assumed Unix delete-while-open semantics (os.Remove succeeds on open files) which do not hold on Windows, causing the Acceptance Tests (windows) CI job to fail; replaced with a simpler, cross-platform mode check Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com> Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/8566da0c-034e-4a52-ab7d-b979fbf37fd3
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…skfile.dev from flaky link check - Add log.Warn() in applyGlobCacheConfig when ATMOS_FS_GLOB_CACHE_TTL or ATMOS_FS_GLOB_CACHE_MAX_ENTRIES are below their minimum thresholds and clamped up — satisfies the "log when clamped" requirement from the problem statement - Add taskfile.dev to lychee.toml exclude list to prevent spurious CI failures due to intermittent connection resets from GitHub Actions IP ranges (site is valid but often blocks/resets CI connections) Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/3cdca293-f7f6-4b06-9fd9-221f26bef085 Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation