Conversation
5.4-mini pass. Build + tests clean first try. - parser.go: +274 lines covering playbook/inventory surface - types.go: +10 lines of new DTOs - executor.go, local_client.go: aligning call sites - parser_test.go: +73 lines coverage Co-Authored-By: Virgil <virgil@lethean.io>
- go.mod: module dappco.re/go/core/ansible → dappco.re/go/ansible
- go.mod requires: dappco.re/go/core/{io,log} → dappco.re/go/{io,log}
- Removed stale forge.lthn.ai/core/go-log indirect (maps to dappco.re/go/log)
- 7 *.go files updated with import rewrites + CLI self-import
Closes tasks.lthn.sh/view.php?id=421
Co-authored-by: Codex <noreply@openai.com>
- Bump dappco.re/go/* deps to v0.8.0-alpha.1 in go.mod (any forge.lthn.ai/core/* paths migrated to canonical dappco.re/go/* form) - Add tests/cli/ansible/Taskfile.yaml AX-10 scaffold (build/vet/test under default deps), per RFC-CORE-008-AGENT-EXPERIENCE.md §10 Co-Authored-By: Athena <athena@lthn.ai>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis pull request introduces async execution capabilities for tasks with optional polling, adds comprehensive template expression evaluation and filtering, enhances the parser with configurable storage mediums and new high-level APIs, refactors condition evaluation logic, and reorganises the module structure by consolidating dependencies into standalone packages. It also introduces a new Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
local_client.go (1)
84-159:⚠️ Potential issue | 🟠 MajorError handling violates coding guidelines.
Multiple functions return errors without wrapping them using
coreerr.E()from thego-logpackage:
- Line 87:
return errfromio.ReadAll- Line 91:
return errfromos.MkdirAll- Line 93:
return os.WriteFile(...)(error not wrapped)- Line 97:
return os.ReadFile(remote)(error not wrapped)- Line 108:
return false, errfromos.Stat- Line 117:
return nil, errfromos.Stat- Line 139:
return "", "", -1, stdinErrfromcmd.StdinPipe- Line 158:
return stdout, stderr, -1, errfromcmd.RunAll errors in production code must be wrapped with
coreerr.E(scope, message, err)to provide proper error context and tracing.As per coding guidelines: all errors in production code must use
coreerr.E(scope, message, err)fromgo-log, neverfmt.Errorf.Example fix for Upload method
func (c *localClient) Upload(_ context.Context, localReader io.Reader, remote string, mode os.FileMode) error { content, err := io.ReadAll(localReader) if err != nil { - return err + return coreerr.E("local_client.upload", "failed to read upload content", err) } if err := os.MkdirAll(filepath.Dir(remote), 0o755); err != nil { - return err + return coreerr.E("local_client.upload", "failed to create directory", err) } - return os.WriteFile(remote, content, mode) + if err := os.WriteFile(remote, content, mode); err != nil { + return coreerr.E("local_client.upload", "failed to write file", err) + } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local_client.go` around lines 84 - 159, Wrap all returned errors in the shown functions with coreerr.E to add scope and context: in Upload, wrap errors from io.ReadAll, os.MkdirAll and os.WriteFile using coreerr.E("localClient", "read/upload file", err) (or similar messages per failure); in Download wrap os.ReadFile errors with coreerr.E("localClient", "read remote file", err); in FileExists and Stat wrap os.Stat errors with coreerr.E("localClient", "stat path", err) and return the wrapped error; in runLocalShell wrap the stdin pipe error and the final cmd.Run error with coreerr.E("localClient", "run local shell", err) (or distinct messages like "open stdin" and "execute command") so every error returned uses coreerr.E(scope, message, err) instead of returning raw err.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@async_features.go`:
- Around line 23-29: The current code in launchDetachedAsyncTask creates a timed
context with context.WithTimeout and calls defer cancel() in the parent
function, which cancels the async context before the launched goroutine starts;
fix by creating the timeout context inside the goroutine (or move the
cancel/defer into the goroutine) so the goroutine owns the context lifetime and
calls cancel() when it finishes; update both places where WithTimeout is used
(the block around asyncCtx, cancel := context.WithTimeout(...) and the second
occurrence at lines 38-40) to ensure cancel() is not deferred in the parent but
invoked by the goroutine that uses asyncCtx.
- Around line 48-61: The cloned Executor currently reuses the parent clients map
via cloneClientMap(e.clients), which shares sshExecutorClient instances and can
leak per-task become/sudo state; change the clone logic in the Executor creation
so that clients is initialized to a fresh map and each entry is created as a new
sshExecutorClient instance (copying only safe immutable connection/config fields
from the original client) instead of copying the existing client objects; update
or replace cloneClientMap usage to a function that returns a new map of newly
constructed sshExecutorClient objects (preserving connection config but not any
task-level state) so async clones do not share client state with the parent.
In `@executor.go`:
- Around line 2870-2874: The current guard logic treats unresolved operands as
satisfying the comparison by returning (true, true); change it so that if either
e.resolveConditionOperandValue(left, ...) or
e.resolveConditionOperandValue(right, ...) returns !OK, the comparison fails
closed by returning (false, true) instead. Update the if !leftOK || !rightOK
block in the comparison code path to return the non-satisfied result (false,
true) so parsed operators don't erroneously evaluate as true when operands are
missing.
- Around line 2901-2959: splitBinaryCondition currently requires
isConditionBoundary around the operator for all ops, which prevents matching
symbolic operators like <=, !=, >, etc.; modify the logic so only word-like
operators (e.g., "in", "not in", "contains") require surrounding token
boundaries. Inside splitBinaryCondition (before the checks that use
isConditionBoundary for cond[i-1] and cond[end]), compute a flag (e.g.,
isWordOp) that is true when op contains any ASCII letters or underscore and
false for purely symbolic operators, then only perform the left/right boundary
checks if isWordOp is true; keep the existing trimming and empty operand checks
and return behavior unchanged for other cases.
In `@go.mod`:
- Around line 7-8: The go.mod now references modules dappco.re/go/io and
dappco.re/go/log but go.sum lacks their checksum entries; run `go mod tidy` (or
`go get dappco.re/go/io@v0.8.0-alpha.1 dappco.re/go/log@v0.8.0-alpha.1` then `go
mod tidy`) to fetch modules and update go.sum so the build/typecheck succeeds,
then commit the updated go.sum alongside the modified go.mod.
In `@parser.go`:
- Around line 22-25: Parser.medium is currently unsynchronized and can be
written by SetMedium while being read by readFile, exists, isDir, and listDir,
causing data races; either make the medium immutable after construction (remove
SetMedium) or protect access with a concurrency primitive. Fix by adding a
sync.Mutex or sync.RWMutex (or an atomic.Value) to Parser and use it in
SetMedium to guard writes and in readFile, exists, isDir, and listDir to guard
reads (or store the medium once during construction and remove SetMedium to
enforce immutability); update the Parser struct to include the mutex/atomic and
wrap all accesses to Parser.medium accordingly to eliminate the race.
- Around line 386-396: Parser.expandFilePattern currently uses filepath.Glob
which only works on the local filesystem; when a non-local coreio.Medium is set
(via SetMedium) wildcard patterns in ParseVarsFiles bypass the medium. Update
expandFilePattern to detect wildcard patterns and if p.medium is non-local
either (a) call a globbing/listing method on the medium (e.g. a hypothetical
p.medium.Glob or p.medium.ReadDir/Walk to match pattern) and return those
matches, or (b) reject wildcard patterns by returning a clear error when
p.medium is not the local filesystem; keep the existing filepath.Glob path only
when the medium is nil/local, and ensure subsequent reads still use p.readFile
so non-local media are consistently used.
In `@template_features.go`:
- Around line 44-55: The code only checks registered results when expr contains
a dot, causing bare names like "{{ cmd_result }}" to fall through; update the
resolution logic in the template lookup to consult e.getRegisteredVar for the
bare expr before calling lookupExprValue: after the dotted-path branch, call
e.getRegisteredVar(host, expr) and if non-nil return that whole result (true) so
whole-result templating and filters on registered vars are preserved; ensure
this check happens prior to invoking lookupExprValue(expr, host, task).
---
Outside diff comments:
In `@local_client.go`:
- Around line 84-159: Wrap all returned errors in the shown functions with
coreerr.E to add scope and context: in Upload, wrap errors from io.ReadAll,
os.MkdirAll and os.WriteFile using coreerr.E("localClient", "read/upload file",
err) (or similar messages per failure); in Download wrap os.ReadFile errors with
coreerr.E("localClient", "read remote file", err); in FileExists and Stat wrap
os.Stat errors with coreerr.E("localClient", "stat path", err) and return the
wrapped error; in runLocalShell wrap the stdin pipe error and the final cmd.Run
error with coreerr.E("localClient", "run local shell", err) (or distinct
messages like "open stdin" and "execute command") so every error returned uses
coreerr.E(scope, message, err) instead of returning raw err.
🪄 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: 77ee45dc-122a-4755-be5d-382549cb064f
📒 Files selected for processing (14)
async_features.gocmd/ansible/ansible.goexecutor.goexecutor_extra_test.gogo.modlocal_client.gomodules.goparser.goparser_test.gossh.gotemplate_features.gotest_primitives_test.gotests/cli/ansible/Taskfile.yamltypes.go
12 files modified, +265/-41. All actionable findings dispositioned. Code fixes: - async_features.go + executor.go: async timeout context lifecycle (was cancelled before goroutine use) - async_features.go: detached async clone no longer reuses parent client cache - parser.go: Parser.medium access now synchronised - parser.go: wildcard vars files honour non-local medium routing - executor.go: bare registered result lookup regression closed - local_client.go: errors are wrapped instead of raw - template_features.go: missing condition operands no longer evaluate to true; symbolic condition operators now require spaces - go.mod + go.sum: dappco.re/go/io + log dependency download + checksum entries Doc fixes: - AX-2 docstrings on exported funcs to clear CodeRabbit coverage warning (templateValueGreater, decodeInventoryHostVarsValue, etc.) Disposition replies (no code change): - SonarCloud PR/branch issues: API returned 0 unresolved for dAppCore_go-ansible PR 4 / dev — RESOLVED-COMMENT - GHAS/Dependabot/secret scanning: 0 open alerts, secret scanning disabled, code-scanning API unavailable to token — RESOLVED-COMMENT Verification: gofmt -l clean, git diff --check clean. GOWORK=off + explicit GOPATH/GOMODCACHE/GOCACHE go vet + go test -count=1 ./... clean. No golangci-lint config in repo. Closes findings on #4 Co-authored-by: Codex <noreply@openai.com>
Brings exported-surface docstring coverage on the codex-pushed PR #4 delta (commit d7d6e33) from 80% to 100%. - executor.go: environmentSSHClient.Run + RunScript - local_client.go: BecomeState, SetBecome, Close, Run, RunScript, Upload, Download, FileExists, Stat (9 methods) Pure docs. No behaviour change. gofmt-clean. Co-authored-by: Hephaestus <hephaestus@cladius>
|
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Snider
added a commit
that referenced
this pull request
Apr 27, 2026
Round 2 verification on PR #4. CodeRabbit re-flagged previously-fixed items; verified each is still present from d7d6e33: - async_features.go: timeout context lifecycle (line 25) - async_features.go: cached clients recreated not shared (line 79) - executor.go: unresolved operands fail closed (line 2883) - executor.go: symbolic operators with spaces (line 2914) - parser.go: sync.RWMutex on medium access (line 24) - parser.go: non-local wildcard vars rejected with coreerr.E (line 421) - template_features.go: bare registered-result lookup (line 55) - local_client.go: errors wrapped with coreerr.E (line 111) - go.mod / go.sum: replacements produce replacement-path sums Only diff this pass: gofmt on cmd/ansible/ansible.go (single line). Verification: gofmt -l clean, GOWORK=off go vet + go test -count=1 ./... pass with explicit cache paths. Co-authored-by: Codex <noreply@openai.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
2 participants
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.
Routine dev→main PR opened by Hephaestus PR-cadence task.
This PR exists to:
ahead_by: 4 commit(s) (per gh api compare)If CodeRabbit clears with no blocking comments, this PR is eligible for
gh pr merge --merge(real merge commit, no force-push). Conflicts and review feedback should be addressed on the dev branch before merge.Co-authored-by: Hephaestus hephaestus@cladius
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores