Open
Conversation
… with proper deps
…althy, STAGING_CONTAINER var, test-guards.sh
… native/e2e split
…est:e2e→test:devcontainer, install expect
… guard checks, expect tool, unified lifecycle
…d, rewrite test-react-build.sh with expect/fallback
…ng for @splunk/create validation
…@splunk/create wizard
…t:create expect script
…reate expect script
…eact:create expect script
…(not nested packages/packages/)
…g cleans stale volumes, robust pre/post cleanup
…app, test-react-app, testReactApp)
Integrate Splunk UCC (Universal Configuration Console) framework into the dev container template, enabling declarative add-on development from globalConfig.json with auto-generated UI, REST handlers, and conf files. Changes: - Add ucc-gen (splunk-add-on-ucc-framework) to post-create.sh pip install - Add 5 new Taskfile tasks: ucc:init, ucc:build, ucc:link, ucc:dev, ucc:package - Add UCC output bind-mount in docker-compose.yml (ucc/output → /opt/splunk/ucc-apps) - Update stage:install to handle .tar.gz files (UCC package format) - Add ucc-lifecycle test suite (23 checks) to E2E tests - Add anthropic.claude-code and mutantdino.resourcemonitor VS Code extensions - Add Claude Code memory persistence via symlink in post-create.sh - Persist .claude/memory/ in project directory (survives container rebuilds) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cleanup was deleting ucc/output/ directory (rm -rf), breaking the Docker bind mount for subsequent UCC tests. Changed to rm contents only (ucc/output/*). Also handle npx "Ok to proceed?" prompt in react-create expect script. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR comprehensively expands the development environment with AI-assisted context (Claude memory files), detailed architectural specifications for task orchestration and testing improvements, refactors the Taskfile from imperative command chains to declarative dependency models with internal guard tasks, extends devcontainer tooling with Claude Code and Expect automation, and introduces end-to-end test coverage for UCC (Splunk Add-on UCC Framework) integration and lifecycle validation. Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR encompasses a multi-system refactor with high complexity and substantial heterogeneity. The Taskfile refactoring alone (309 lines) introduces intricate guard patterns, dependency chains, and staging pipeline decomposition requiring careful validation. Test infrastructure changes span multiple new scripts and substantial modifications to existing ones. While many individual documentation/memory files are straightforward additions, the overall scope, interplay between task orchestration changes and test expectations, and Python script extensions demand thorough cross-component reasoning. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Taskfile.yml (2)
332-353:⚠️ Potential issue | 🟠 MajorDon't make
app:createfail after the scaffold already succeeded.Once
dev:ensure-linksgained__dev:ensure-running, a stopped dev container turnstask app:create APP_NAME=xinto a non-zero command after the app directory is already created. These post-scaffold sync steps should be conditional or best-effort so offline creation still succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Taskfile.yml` around lines 332 - 353, The app:create task performs the scaffold but then runs post-scaffold steps (dev:ensure-links and dev:refresh) which can fail when the dev container is stopped; make those follow-up steps best-effort so scaffold success isn’t turned into a failure. Update the app:create task to run dev:ensure-links and dev:refresh conditionally or mark them to ignore failures (e.g., add ignore_error: true to the dev:ensure-links invocation or wrap both calls with a runtime check that __dev:ensure-running or the dev container is reachable) so that creating the app directory always returns success even if these sync tasks fail.
705-715:⚠️ Potential issue | 🟡 Minor
DC_CLIis unused by the child script.The variable is set locally in the Taskfile but never exported or passed to
bash tests/e2e/devcontainer-test.sh. The test script performs its own redundant detection logic (lines 30-34), creating a separateDEVCONTAINERvariable instead of usingDC_CLI. To eliminate duplication, either exportDC_CLIand modify the test script to use it, or remove the detection logic from Taskfile.yml and rely on the test script's own detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Taskfile.yml` around lines 705 - 715, DC_CLI is set in the Taskfile but never consumed by the test script; update the flow so the child script uses the CLI chosen by the Taskfile. Export DC_CLI from the Taskfile (so it's available in the environment) and modify tests/e2e/devcontainer-test.sh to prefer the DC_CLI env var (or use its own detection only if DC_CLI is unset), replacing or removing the redundant DEVCONTAINER detection logic; reference the DC_CLI variable in the script and keep the fallback behavior for compatibility.
🧹 Nitpick comments (4)
splunk/scripts/deps-install.py (1)
248-249: Guard task dependency is correctly documented, but consider defensive coding for direct invocation.The comment accurately documents that
__dev:ensure-runningvalidates the container precondition. However, if someone invokes this script directly (bypassing the Taskfile),find_container()could return an empty string, and subsequentdocker_exec()calls would fail with confusing errors.Consider adding a minimal defensive check for direct invocation scenarios:
🛡️ Optional: Add defensive check for direct script invocation
# Find container (guard task __dev:ensure-running already verified it's running) container = find_container(args.compose_file, args.env_file, args.service) + if not container: + print("ERROR: Container not found. Run via 'task deps:install' or ensure container is running.") + return 1 os.makedirs(args.stage_dir, exist_ok=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@splunk/scripts/deps-install.py` around lines 248 - 249, The code assumes find_container(args.compose_file, args.env_file, args.service) always returns a container but that may be falsey when the script is invoked directly; add a defensive check after the call that verifies the returned container (variable container) is non-empty and, if empty, logs a clear error via the same logger or prints a message and exits with a non-zero status before calling docker_exec or other functions that require a valid container ID; reference find_container, container, and docker_exec to locate where to insert this check..windloop/specs/taskfile-dependency-refactor/progress.txt (1)
5-5: Minor inconsistency in summary.The summary states "E2E verification pending" but all tasks (including 7.5 which adds E2E test coverage) are marked DONE. Consider updating the summary to reflect completion.
📝 Suggested fix
-# SUMMARY: 6/6 done | E2E verification pending (requires running containers) +# SUMMARY: ALL DONE | 18 tasks completed including E2E verification🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.windloop/specs/taskfile-dependency-refactor/progress.txt at line 5, The SUMMARY line currently reads "# SUMMARY: 6/6 done | E2E verification pending (requires running containers)" which is inconsistent because task 7.5 (E2E coverage) is marked DONE; update that summary string to reflect completion (for example change it to "# SUMMARY: 6/6 done | E2E verification complete" or "# SUMMARY: 6/6 done | All checks complete") so the progress.txt summary matches the task statuses..claude/memory/research_ucc_framework.md (1)
13-22: Add language specifier to code block for linter compliance.Similar to the REST connector doc, the directory structure code block should have a language specifier.
📝 Proposed fix
### Source structure (developer writes): -``` +```text my_addon/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/memory/research_ucc_framework.md around lines 13 - 22, The directory tree code block in research_ucc_framework.md is missing a language specifier; change the opening triple backticks for the block that starts with "my_addon/" to include a language (e.g., "text") so the linter accepts it—update the code fence in the code block showing the directory structure in .claude/memory/research_ucc_framework.md to use ```text instead of ``` while leaving the contents unchanged..claude/memory/research_rest_connector.md (1)
23-31: Add language specifier to code block for linter compliance.The static analysis tool flagged the missing language specifier. For directory structure listings, you can use
textor an empty specifier with triple backticks.📝 Proposed fix
### Python structure: -``` +```text bin/rest_api_input.py # BaseModInput subclass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/memory/research_rest_connector.md around lines 23 - 31, The fenced code block containing the directory listing (the block showing bin/rest_api_input.py, bin/rest_api_lib/, etc.) is missing a language specifier; update the opening fence from ``` to ```text (or simply include an explicit language like text) in .claude/memory/research_rest_connector.md so the linter recognizes the block as plain text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/post-create.sh:
- Around line 35-37: The symlink creation using ln -sfn will fail if
"$HOME/.claude/projects/-workspace/memory" exists as a real directory; update
the post-create script to ensure the parent dir exists (mkdir -p
"$HOME/.claude/projects/-workspace") and explicitly remove any pre-existing real
directory at "$HOME/.claude/projects/-workspace/memory" (e.g., check and rm -rf
if it's a directory and not a symlink) before running ln -sfn
/workspace/.claude/memory "$HOME/.claude/projects/-workspace/memory" so the
symlink is created reliably.
In @.windloop/specs/taskfile-dependency-refactor/design.md:
- Around line 36-39: The diagram still lists the old dependency token
__app:resolve for AppCreate; update the subgraph entry
AppCreate["app:create<br/>deps: __app:resolve"] to reference the new dependency
__ensure-app-name (e.g., AppCreate["app:create<br/>deps: __ensure-app-name"]) so
the visual graph matches the implementation and the APP_NAME section.
In @.windloop/specs/taskfile-dependency-refactor/requirements.md:
- Around line 24-25: The requirements document defines conflicting contracts for
stage:install—one says it should fail via the guard task __stage:ensure-running
with a "Staging container is not running. Start with: task stage:up" message,
while another says stage:install should depend on or auto-start the staging
container via stage:up; pick one consistent behavior and update the spec
accordingly: either (A) keep the guard __stage:ensure-running and state that
stage:install SHALL invoke the guard and fail with the exact message (update any
references to "depend on stage:up" to reflect failure), or (B) change
__stage:ensure-running to instead trigger/await stage:up and state that
stage:install SHALL depend on or auto-start stage:up (remove the failing-message
requirement); ensure all mentions of stage:install, __stage:ensure-running, and
stage:up in the document are updated to the chosen contract so there is a
single, unambiguous acceptance criterion.
In @.windloop/specs/test-improvements/design.md:
- Around line 13-27: Update the lifecycle design doc to include the new UCC
suite: change the description of run-lifecycle.sh from a "7-suite runner" to an
"8-suite runner", add tests/e2e/test-ucc-lifecycle.sh to the listed test files,
and update the execution graph so the flow continues past test-staging.sh to the
new test-ucc-lifecycle.sh; also ensure Taskfile.yml references are consistent
with the new suite count and adjust any other occurrences (e.g., lines 40-49)
that still show the old seven-suite layout.
In `@Taskfile.yml`:
- Around line 392-412: The __react:resolve task currently writes REACT_APP_NAME
from {{.APP_NAME}} even when REACT_PATH is discovered by falling back to the
first package, causing a mismatch between REACT_APP_NAME and REACT_APP_PATH;
update __react:resolve (and its use of __ensure-app-name) to either fail fast
when APP_NAME does not actually resolve to the discovered REACT_PATH or derive
REACT_APP_NAME from the resolved path (e.g., the package directory name) before
writing .task/react-env; ensure you reference and fix the variables REACT_PATH,
REACT_APP_NAME and APP_NAME within __react:resolve so they are consistent
(either abort with an explanatory error or set REACT_APP_NAME to the resolved
package name).
- Around line 541-560: Currently __ucc:resolve auto-detects the first directory
with globalConfig.json even when APP_NAME is provided; change the logic so that
if APP_NAME is set (non-empty) you only accept
UCC_PATH="${UCC_SRC_DIR}/${APP_NAME}" and error if that directory or its
globalConfig.json is missing, and only perform the auto-detect fallback when
APP_NAME is empty; modify the block that computes UCC_PATH/UCC_ADDON (variables
UCC_PATH, UCC_ADDON, UCC_SRC_DIR, APP_NAME) in the __ucc:resolve target so it
fails fast for a mismatched APP_NAME instead of silently picking another addon.
- Around line 251-265: The loop in stage:install iterates all historical *.tgz
and *.tar.gz files (TGZ) causing older artifacts to be reinstalled; replace the
glob loop with logic that selects only the intended artifact (e.g. the
most-recent build) before installing. Concretely, change the TGZ loop to compute
a single file (e.g. TGZ=$(ls -t {{.STAGE_DIR}}/*.tgz {{.STAGE_DIR}}/*.tar.gz
2>/dev/null | head -n1) && [ -f "$TGZ" ] || continue) and then run the existing
docker exec install command against that single TGZ (keeping INSTALLED/SKIPPED
handling), and apply the same single-file-selection fix to the duplicate block
referenced at 663-675.
- Around line 614-615: The current command assigns VERSION from a pipeline where
sed succeeds on empty input, so the || fallback never runs; change the command
so the fallback applies to git describe before piping to sed—for example, run
git describe --tags --abbrev=0 (with its stderr redirected) OR echo "1.0.0"
first, then pipe that result into sed 's/^v//' and assign to VERSION; update the
line referencing VERSION to use this grouped command so untagged repos get the
default "1.0.0".
In `@tests/e2e/helpers.sh`:
- Line 9: The default value for the TEST_REACT_APP environment variable uses
camelCase which is inconsistent with TEST_CMD_APP and Splunk conventions; update
the fallback in tests/e2e/helpers.sh so
TEST_REACT_APP="${TEST_REACT_APP:-test_react_app}" (snake_case, lowercase with
underscore) to match TEST_CMD_APP and the established app-naming pattern used in
cleanup and error paths.
In `@tests/e2e/run-lifecycle.sh`:
- Around line 23-30: The cleanup is too broad: restrict deletions to test
fixtures and safely preserve/restore APP_NAME from .env; replace hard rm -rf of
react/packages and react/package.json with removals of dedicated test fixture
paths (e.g., instead of react/packages use react/test-fixtures/packages or the
specific test dirs like ucc/test_ucc_addon and splunk/stage), and before
modifying .env capture APP_NAME into a variable (e.g., SAVED_APP_NAME="$(grep
'^APP_NAME=' .env || true)"), remove the key safely (sed -i.bak '/^APP_NAME=/d'
.env), and register a trap that on EXIT restores the saved APP_NAME back into
.env (or removes the backup) so real developer files are not lost; update
run-lifecycle.sh to operate only on these test-specific paths and add the trap
restore logic referencing .env, APP_NAME, and trap.
In `@tests/e2e/test-guards.sh`:
- Around line 28-45: The test deletes APP_NAME from .env but only restores it at
the end, so add a safe cleanup using a trap to always restore the original value
and remove the .env.bak on exit; capture the original value in SAVED_APP_NAME as
already done, then install a trap on EXIT (or ERR+EXIT) that, if SAVED_APP_NAME
is non-empty, appends it back into .env and removes .env.bak (and also remove
any temporary .env if created), ensuring this restoration runs regardless of
early exits from the task app:create check.
In `@tests/e2e/test-react-build.sh`:
- Around line 33-58: The pipeline command substitutions for REACT_APP_DIR and
DETECTED_APP_NAME can fail under set -euo pipefail and must be made non-fatal;
append "|| true" to the find/grep/cut substitutions that set REACT_APP_DIR and
DETECTED_APP_NAME, and after any fail call in this block (the branches that call
fail "react:create did not create app directory" and fail "react:create did not
sync APP_NAME in .env") add an explicit "exit 1" so the script exits immediately
instead of reaching cleanup. Also harden cleanup/cleanup checks around
STAGE_DIR: ensure you only rm -rf when REACT_APP_DIR is non-empty and STAGE_DIR
is an actual directory under that path (e.g., test -n "$REACT_APP_DIR" && [ -d
"$STAGE_DIR" ]) to avoid expanding to "/stage" and deleting the wrong target;
apply the same non-fatal substitution and exit pattern to the cleanup code
referenced (lines that reference STAGE_DIR).
In `@tests/e2e/test-staging.sh`:
- Around line 26-30: Replace the fixed "sleep 5" with a retry/poll loop that
calls the existing check_app_rest helper until it returns success or a sane
timeout is reached (e.g., total wait ~60s with short intervals), so the script
waits for /services/apps/local to reflect the CLI-installed app instead of
flaking on slow hosts; update both occurrences (the check after CLI install and
the other occurrence noted) to use this polling pattern and ensure the loop
exits non-zero on timeout to surface failures.
- Around line 32-35: The assignment to REACT_APP_NAME using grep can cause the
script to exit under set -euo pipefail when .env is missing or APP_NAME isn't
found, making the fallback unreachable; update the REACT_APP_NAME assignment
(the command substitution that runs grep "^APP_NAME=" .env | cut -d= -f2) so it
never returns a failing status (e.g., guard with a file-exists check or append
`|| true`/use `|| echo ""` to swallow non-zero exit) and then keep the existing
fallback that sets REACT_APP_NAME="${TEST_REACT_APP}" if empty.
In `@tests/e2e/test-ucc-lifecycle.sh`:
- Around line 11-12: The assertions use raw TEST_UCC_APP; compute the expected
input filenames using the same normalization logic as ucc:init: replace '-' with
'_' and, if the name ends with '_input', strip that suffix, then append '_input'
to form the expected filename. Update all places that build expected filenames
from TEST_UCC_APP (references to TEST_UCC_APP in the assertions around the
Taskfile checks) to perform this normalization before comparing, so values like
"my-addon" or "foo_input" produce the correct expected names.
- Around line 148-153: Decide whether OpenAPI generation is required or
optional, then update the check accordingly: if it's optional, replace the fail
call in the openapi.json existence block with a non-failing warning/skip (e.g.,
call warn/skip or echo a warning) and keep the "non-critical" wording; if it's
required, keep the fail behavior but remove the "(non-critical)" text from the
message so the failure's severity is consistent; locate the check that tests for
"${BUILD_DIR}/appserver/static/openapi.json" and adjust the pass/fail call
there.
- Around line 11-17: The test mutates APP_NAME and leaves generated artifacts
and .env in place; add setup/teardown to backup and restore .env and to remove
ucc/${TEST_UCC_APP}, ucc/output/${TEST_UCC_APP}, and any staged tarballs before
and after the suite. Specifically, save ORIGINAL_ENV=$(cat .env) or copy .env,
register a trap to restore the .env and delete the directories and tarballs on
EXIT, and ensure the task invocation (task ucc:init using TEST_UCC_APP) runs
between the backup and the cleanup so repeated runs start from a clean state;
apply the same pattern around the other affected blocks (the ranges noted) to
avoid leaking state across suites.
- Around line 17-23: The current checks capture command output into OUTPUT after
running task but do not capture the exit status, which under set -euo pipefail
can cause the script to exit before fail()/results() run; change each pattern
that does OUTPUT=$(task ...) followed by if echo "$OUTPUT" | grep -q ... to the
form that captures both output and exit status by performing the command inside
the if (e.g., if OUTPUT=$(task ucc:init APP_NAME="${TEST_UCC_APP}" 2>&1); then
... else fail ...; echo " Output: ${OUTPUT:0:300}" fi) for all occurrences
mentioned (the ucc:init check using OUTPUT, the other task invocations at the
other noted locations, and the check_app_rest invocation) so fail() bookkeeping
and results() still run on command failures.
---
Outside diff comments:
In `@Taskfile.yml`:
- Around line 332-353: The app:create task performs the scaffold but then runs
post-scaffold steps (dev:ensure-links and dev:refresh) which can fail when the
dev container is stopped; make those follow-up steps best-effort so scaffold
success isn’t turned into a failure. Update the app:create task to run
dev:ensure-links and dev:refresh conditionally or mark them to ignore failures
(e.g., add ignore_error: true to the dev:ensure-links invocation or wrap both
calls with a runtime check that __dev:ensure-running or the dev container is
reachable) so that creating the app directory always returns success even if
these sync tasks fail.
- Around line 705-715: DC_CLI is set in the Taskfile but never consumed by the
test script; update the flow so the child script uses the CLI chosen by the
Taskfile. Export DC_CLI from the Taskfile (so it's available in the environment)
and modify tests/e2e/devcontainer-test.sh to prefer the DC_CLI env var (or use
its own detection only if DC_CLI is unset), replacing or removing the redundant
DEVCONTAINER detection logic; reference the DC_CLI variable in the script and
keep the fallback behavior for compatibility.
---
Nitpick comments:
In @.claude/memory/research_rest_connector.md:
- Around line 23-31: The fenced code block containing the directory listing (the
block showing bin/rest_api_input.py, bin/rest_api_lib/, etc.) is missing a
language specifier; update the opening fence from ``` to ```text (or simply
include an explicit language like text) in
.claude/memory/research_rest_connector.md so the linter recognizes the block as
plain text.
In @.claude/memory/research_ucc_framework.md:
- Around line 13-22: The directory tree code block in research_ucc_framework.md
is missing a language specifier; change the opening triple backticks for the
block that starts with "my_addon/" to include a language (e.g., "text") so the
linter accepts it—update the code fence in the code block showing the directory
structure in .claude/memory/research_ucc_framework.md to use ```text instead of
``` while leaving the contents unchanged.
In @.windloop/specs/taskfile-dependency-refactor/progress.txt:
- Line 5: The SUMMARY line currently reads "# SUMMARY: 6/6 done | E2E
verification pending (requires running containers)" which is inconsistent
because task 7.5 (E2E coverage) is marked DONE; update that summary string to
reflect completion (for example change it to "# SUMMARY: 6/6 done | E2E
verification complete" or "# SUMMARY: 6/6 done | All checks complete") so the
progress.txt summary matches the task statuses.
In `@splunk/scripts/deps-install.py`:
- Around line 248-249: The code assumes find_container(args.compose_file,
args.env_file, args.service) always returns a container but that may be falsey
when the script is invoked directly; add a defensive check after the call that
verifies the returned container (variable container) is non-empty and, if empty,
logs a clear error via the same logger or prints a message and exits with a
non-zero status before calling docker_exec or other functions that require a
valid container ID; reference find_container, container, and docker_exec to
locate where to insert this check.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3b9cec3-0b0b-4e31-b50a-05adcf192cd4
📒 Files selected for processing (33)
.claude/memory/MEMORY.md.claude/memory/feedback_persist_memory.md.claude/memory/feedback_worktrees.md.claude/memory/project_goals.md.claude/memory/research_rest_connector.md.claude/memory/research_ucc_framework.md.claude/memory/research_ucc_generator_details.md.claude/memory/user_profile.md.devcontainer/devcontainer.json.devcontainer/docker-compose.yml.devcontainer/post-create.sh.gitignore.windloop/specs/taskfile-dependency-refactor/design.md.windloop/specs/taskfile-dependency-refactor/progress.txt.windloop/specs/taskfile-dependency-refactor/requirements.md.windloop/specs/taskfile-dependency-refactor/tasks.md.windloop/specs/test-improvements/design.md.windloop/specs/test-improvements/progress.txt.windloop/specs/test-improvements/requirements.md.windloop/specs/test-improvements/tasks.mdARCHITECTURE.mdREADME.mdTaskfile.ymlsplunk/scripts/deps-install.pytests/e2e/devcontainer-test.shtests/e2e/expect/react-create.exptests/e2e/helpers.shtests/e2e/run-lifecycle.shtests/e2e/test-guards.shtests/e2e/test-react-build.shtests/e2e/test-staging.shtests/e2e/test-ucc-lifecycle.shucc/.gitkeep
Comment on lines
+35
to
+37
| if [ -d /workspace/.claude/memory ]; then | ||
| mkdir -p "$HOME/.claude/projects/-workspace" | ||
| ln -sfn /workspace/.claude/memory "$HOME/.claude/projects/-workspace/memory" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does GNU coreutils ln -sfn SOURCE TARGETreplaceTARGETwhenTARGET already exists as a real directory, or does it create the symlink inside that directory?
💡 Result:
No, GNU coreutils ln -sfn SOURCE TARGET does not replace TARGET when TARGET already exists as a real directory. Instead, with -n (or -T), it treats the TARGET as a normal file. Since -f is specified, it attempts to remove the existing TARGET file before creating the symlink. However, ln refuses to overwrite or remove directories, issuing an error like "cannot overwrite directory" and failing. To replace a real directory with a symlink, the directory must first be removed manually (e.g., rm -rf TARGET), as ln's -f uses unlink which does not support directories on most systems, and coreutils ln explicitly checks and prevents overwriting directories.
Citations:
- 1: https://www.gnu.org/software/coreutils/manual/html_node/ln-invocation.html
- 2: https://manpages.debian.org/bullseye/coreutils/ln.1.en.html
- 3: https://stackoverflow.com/questions/38094453/ln-s-and-overwriting-a-physical-directory
- 4: https://unix.stackexchange.com/questions/293521/does-ln-sf-overwrite-existing-files-which-are-only-symbolic-links
- 5: https://github.com/coreutils/coreutils/blob/master/src/ln.c
- 6: https://www.maizure.org/projects/decoded-gnu-coreutils/ln.html
- 7: https://stackoverflow.com/questions/40806543/ln-sf-does-not-overwrite-a-symlink-to-a-directory
- 8: https://serverfault.com/questions/147787/how-to-update-a-symbolic-link-target-ln-f-s-not-working
Add explicit removal for pre-existing real directories before linking.
Line 37 uses ln -sfn, which will fail with an error ("cannot overwrite directory") if "$HOME/.claude/projects/-workspace/memory" already exists as a real directory. While the command won't silently create a nested link, it will still fail, leaving the directory unlinked. Removing any existing real directory first ensures the symlink is properly created.
Proposed fix
if [ -d /workspace/.claude/memory ]; then
mkdir -p "$HOME/.claude/projects/-workspace"
- ln -sfn /workspace/.claude/memory "$HOME/.claude/projects/-workspace/memory"
+ CLAUDE_MEMORY_LINK="$HOME/.claude/projects/-workspace/memory"
+ if [ -e "$CLAUDE_MEMORY_LINK" ] && [ ! -L "$CLAUDE_MEMORY_LINK" ]; then
+ rm -rf "$CLAUDE_MEMORY_LINK"
+ fi
+ ln -sfn /workspace/.claude/memory "$CLAUDE_MEMORY_LINK"
echo "Claude Code memory linked to project directory"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ -d /workspace/.claude/memory ]; then | |
| mkdir -p "$HOME/.claude/projects/-workspace" | |
| ln -sfn /workspace/.claude/memory "$HOME/.claude/projects/-workspace/memory" | |
| if [ -d /workspace/.claude/memory ]; then | |
| mkdir -p "$HOME/.claude/projects/-workspace" | |
| CLAUDE_MEMORY_LINK="$HOME/.claude/projects/-workspace/memory" | |
| if [ -e "$CLAUDE_MEMORY_LINK" ] && [ ! -L "$CLAUDE_MEMORY_LINK" ]; then | |
| rm -rf "$CLAUDE_MEMORY_LINK" | |
| fi | |
| ln -sfn /workspace/.claude/memory "$CLAUDE_MEMORY_LINK" | |
| echo "Claude Code memory linked to project directory" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/post-create.sh around lines 35 - 37, The symlink creation
using ln -sfn will fail if "$HOME/.claude/projects/-workspace/memory" exists as
a real directory; update the post-create script to ensure the parent dir exists
(mkdir -p "$HOME/.claude/projects/-workspace") and explicitly remove any
pre-existing real directory at "$HOME/.claude/projects/-workspace/memory" (e.g.,
check and rm -rf if it's a directory and not a symlink) before running ln -sfn
/workspace/.claude/memory "$HOME/.claude/projects/-workspace/memory" so the
symlink is created reliably.
Comment on lines
+36
to
+39
| subgraph App["App Management"] | ||
| SyncLinks["app:sync-links<br/>deps: __dev:ensure-running"] | ||
| AppCreate["app:create<br/>deps: __app:resolve"] | ||
| end |
There was a problem hiding this comment.
The dependency graph still documents removed __app:resolve.
The implementation now uses __ensure-app-name for app:create, so this diagram is already stale and contradicts the later APP_NAME section in the same doc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.windloop/specs/taskfile-dependency-refactor/design.md around lines 36 - 39,
The diagram still lists the old dependency token __app:resolve for AppCreate;
update the subgraph entry AppCreate["app:create<br/>deps: __app:resolve"] to
reference the new dependency __ensure-app-name (e.g.,
AppCreate["app:create<br/>deps: __ensure-app-name"]) so the visual graph matches
the implementation and the APP_NAME section.
Comment on lines
+24
to
+25
| 1. WHEN any task that requires the dev container (e.g. `dev:refresh`, `dev:restartd`, `dev:ensure-links`, `deps:install`) is run and the dev container is NOT running, THEN the guard task `__dev:ensure-running` SHALL fail with a message like "Dev container is not running. Start with: task dev:up". | ||
| 2. WHEN any task that requires the staging container (e.g. `stage:install`) is run and the staging container is NOT running, THEN the guard task `__stage:ensure-running` SHALL fail with a message like "Staging container is not running. Start with: task stage:up". |
There was a problem hiding this comment.
Requirement 1 and Requirement 3 define two different stage:install contracts.
Lines 24-25 say stage:install should fail via __stage:ensure-running, while Lines 48-51 say it should depend on stage:up and auto-start staging. Only one of those behaviors can be the acceptance criteria.
Also applies to: 48-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.windloop/specs/taskfile-dependency-refactor/requirements.md around lines 24
- 25, The requirements document defines conflicting contracts for
stage:install—one says it should fail via the guard task __stage:ensure-running
with a "Staging container is not running. Start with: task stage:up" message,
while another says stage:install should depend on or auto-start the staging
container via stage:up; pick one consistent behavior and update the spec
accordingly: either (A) keep the guard __stage:ensure-running and state that
stage:install SHALL invoke the guard and fail with the exact message (update any
references to "depend on stage:up" to reflect failure), or (B) change
__stage:ensure-running to instead trigger/await stage:up and state that
stage:install SHALL depend on or auto-start stage:up (remove the failing-message
requirement); ensure all mentions of stage:install, __stage:ensure-running, and
stage:up in the document are updated to the chosen contract so there is a
single, unambiguous acceptance criterion.
Comment on lines
+13
to
+27
| ``` | ||
| tests/e2e/ | ||
| helpers.sh # Shared test helpers (pass/fail/results/check_app_rest) | ||
| run-lifecycle.sh # Lifecycle test runner (7 suites) | ||
| devcontainer-test.sh # Devcontainer static checks + calls run-lifecycle.sh | ||
| test-guards.sh # Guard failure tests | ||
| test-boot.sh # Splunk boot + health | ||
| test-app-lifecycle.sh # app:create, symlinks, package | ||
| test-deps-install.sh # deps:install idempotency | ||
| test-react-build.sh # react:create (expect) + build + package | ||
| test-staging.sh # stage:deploy pipeline | ||
| test-skip-provision.sh # Container restart skips Ansible | ||
| expect/ # Expect scripts for interactive tasks | ||
| react-create.exp # Automates npx @splunk/create wizard | ||
| ``` |
There was a problem hiding this comment.
The lifecycle design is missing the new UCC suite.
This still documents run-lifecycle.sh as a 7-suite runner and the graph stops at test-staging.sh, but the PR adds tests/e2e/test-ucc-lifecycle.sh and Taskfile.yml advertises 8 suites. The design doc should reflect the current execution path.
Also applies to: 40-49
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.windloop/specs/test-improvements/design.md around lines 13 - 27, Update the
lifecycle design doc to include the new UCC suite: change the description of
run-lifecycle.sh from a "7-suite runner" to an "8-suite runner", add
tests/e2e/test-ucc-lifecycle.sh to the listed test files, and update the
execution graph so the flow continues past test-staging.sh to the new
test-ucc-lifecycle.sh; also ensure Taskfile.yml references are consistent with
the new suite count and adjust any other occurrences (e.g., lines 40-49) that
still show the old seven-suite layout.
Comment on lines
+251
to
+265
| stage:install: | ||
| deps: [stage:package, stage:up] | ||
| cmds: | ||
| - docker compose --env-file {{.ENV_FILE}} -f {{.COMPOSE_STAGING_FILE}} up -d | ||
| - | | ||
| echo "Staging Splunk started on port 18000 (Web), 18089 (REST), 18088 (HEC)" | ||
| echo "Follow logs: task stage:logs" | ||
| desc: Start staging Splunk (apps auto-installed from splunk/stage/ on first start) | ||
| echo "Installing apps from {{.STAGE_DIR}}/ into staging..." | ||
| INSTALLED=0; SKIPPED=0 | ||
| for TGZ in {{.STAGE_DIR}}/*.tgz {{.STAGE_DIR}}/*.tar.gz; do | ||
| [ -f "$TGZ" ] || continue | ||
| FILENAME=$(basename "$TGZ") | ||
| echo " Installing $FILENAME..." | ||
| docker exec --user splunk {{.STAGING_CONTAINER}} bash -lc \ | ||
| "{{.SPLUNK_BIN}} install app /tmp/apps/${FILENAME} -update true -auth admin:\$SPLUNK_PASSWORD" \ | ||
| 2>&1 && INSTALLED=$((INSTALLED + 1)) || SKIPPED=$((SKIPPED + 1)) | ||
| done | ||
| echo "Install complete: $INSTALLED installed, $SKIPPED failed" |
There was a problem hiding this comment.
stage:install replays every historical archive in splunk/stage/.
Because this loop installs every *.tgz and *.tar.gz, repeated ucc:package runs will reinstall all older versioned tarballs as well. That makes the final installed version depend on glob order instead of the artifact you just built.
Also applies to: 663-675
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Taskfile.yml` around lines 251 - 265, The loop in stage:install iterates all
historical *.tgz and *.tar.gz files (TGZ) causing older artifacts to be
reinstalled; replace the glob loop with logic that selects only the intended
artifact (e.g. the most-recent build) before installing. Concretely, change the
TGZ loop to compute a single file (e.g. TGZ=$(ls -t {{.STAGE_DIR}}/*.tgz
{{.STAGE_DIR}}/*.tar.gz 2>/dev/null | head -n1) && [ -f "$TGZ" ] || continue)
and then run the existing docker exec install command against that single TGZ
(keeping INSTALLED/SKIPPED handling), and apply the same single-file-selection
fix to the duplicate block referenced at 663-675.
Comment on lines
+32
to
+35
| # Use actual React app name from .env (set by react:create during react-build suite) | ||
| REACT_APP_NAME=$(grep "^APP_NAME=" .env 2>/dev/null | cut -d= -f2) | ||
| if [ -z "$REACT_APP_NAME" ]; then | ||
| REACT_APP_NAME="${TEST_REACT_APP}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
bash -lc 'set -euo pipefail; APP_NAME=$(grep "^APP_NAME=" /dev/null | cut -d= -f2); echo UNREACHED' \
&& echo "unexpected: assignment did not abort" \
|| echo "expected: missing APP_NAME aborts before the fallback branch"Repository: kundeng/splunk-devcontainer-template
Length of output: 256
🏁 Script executed:
cat -n tests/e2e/test-staging.sh | head -50Repository: kundeng/splunk-devcontainer-template
Length of output: 1773
The fallback for REACT_APP_NAME is unreachable if .env is missing or lacks the APP_NAME pattern.
With set -euo pipefail (line 6), the assignment on line 33 exits the script if grep fails (either .env doesn't exist or the APP_NAME= pattern isn't found). The fallback logic on lines 34–36 never executes.
Fix
-REACT_APP_NAME=$(grep "^APP_NAME=" .env 2>/dev/null | cut -d= -f2)
+REACT_APP_NAME=$(grep "^APP_NAME=" .env 2>/dev/null | cut -d= -f2 || true)
if [ -z "$REACT_APP_NAME" ]; then
REACT_APP_NAME="${TEST_REACT_APP}"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Use actual React app name from .env (set by react:create during react-build suite) | |
| REACT_APP_NAME=$(grep "^APP_NAME=" .env 2>/dev/null | cut -d= -f2) | |
| if [ -z "$REACT_APP_NAME" ]; then | |
| REACT_APP_NAME="${TEST_REACT_APP}" | |
| # Use actual React app name from .env (set by react:create during react-build suite) | |
| REACT_APP_NAME=$(grep "^APP_NAME=" .env 2>/dev/null | cut -d= -f2 || true) | |
| if [ -z "$REACT_APP_NAME" ]; then | |
| REACT_APP_NAME="${TEST_REACT_APP}" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test-staging.sh` around lines 32 - 35, The assignment to
REACT_APP_NAME using grep can cause the script to exit under set -euo pipefail
when .env is missing or APP_NAME isn't found, making the fallback unreachable;
update the REACT_APP_NAME assignment (the command substitution that runs grep
"^APP_NAME=" .env | cut -d= -f2) so it never returns a failing status (e.g.,
guard with a file-exists check or append `|| true`/use `|| echo ""` to swallow
non-zero exit) and then keep the existing fallback that sets
REACT_APP_NAME="${TEST_REACT_APP}" if empty.
Comment on lines
+11
to
+12
| TEST_UCC_APP="${TEST_UCC_APP:-test_ucc_addon}" | ||
|
|
There was a problem hiding this comment.
Compute expected input filenames with the same normalization as ucc:init.
Taskfile.yml converts - to _ and strips a trailing _input before appending _input. These assertions use raw TEST_UCC_APP, so valid overrides like my-addon or foo_input will false-fail.
Also applies to: 38-42, 142-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test-ucc-lifecycle.sh` around lines 11 - 12, The assertions use raw
TEST_UCC_APP; compute the expected input filenames using the same normalization
logic as ucc:init: replace '-' with '_' and, if the name ends with '_input',
strip that suffix, then append '_input' to form the expected filename. Update
all places that build expected filenames from TEST_UCC_APP (references to
TEST_UCC_APP in the assertions around the Taskfile checks) to perform this
normalization before comparing, so values like "my-addon" or "foo_input" produce
the correct expected names.
Comment on lines
+11
to
+17
| TEST_UCC_APP="${TEST_UCC_APP:-test_ucc_addon}" | ||
|
|
||
| # ── Phase 1: Offline (init, build, package) ────────────────────────── | ||
|
|
||
| log "UCC init: scaffold add-on" | ||
|
|
||
| OUTPUT=$(task ucc:init APP_NAME="${TEST_UCC_APP}" 2>&1) |
There was a problem hiding this comment.
Restore .env and generated UCC artifacts around this suite.
This test rewrites APP_NAME via task ucc:init and reuses a fixed TEST_UCC_APP, but it never restores .env or removes ucc/${TEST_UCC_APP}, ucc/output/${TEST_UCC_APP}, or staged tarballs. A rerun will fail the first scaffold assertion, and later suites can resolve the wrong app name.
Also applies to: 54-60, 174-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test-ucc-lifecycle.sh` around lines 11 - 17, The test mutates
APP_NAME and leaves generated artifacts and .env in place; add setup/teardown to
backup and restore .env and to remove ucc/${TEST_UCC_APP},
ucc/output/${TEST_UCC_APP}, and any staged tarballs before and after the suite.
Specifically, save ORIGINAL_ENV=$(cat .env) or copy .env, register a trap to
restore the .env and delete the directories and tarballs on EXIT, and ensure the
task invocation (task ucc:init using TEST_UCC_APP) runs between the backup and
the cleanup so repeated runs start from a clean state; apply the same pattern
around the other affected blocks (the ranges noted) to avoid leaking state
across suites.
Comment on lines
+17
to
+23
| OUTPUT=$(task ucc:init APP_NAME="${TEST_UCC_APP}" 2>&1) | ||
| if echo "$OUTPUT" | grep -q "scaffolded"; then | ||
| pass "ucc:init scaffolded ${TEST_UCC_APP}" | ||
| else | ||
| fail "ucc:init did not scaffold" | ||
| echo " Output: ${OUTPUT:0:300}" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/e2e/test-ucc-lifecycle.sh | head -50Repository: kundeng/splunk-devcontainer-template
Length of output: 1968
🏁 Script executed:
grep -n "set -e" tests/e2e/test-ucc-lifecycle.shRepository: kundeng/splunk-devcontainer-template
Length of output: 99
🏁 Script executed:
cat -n tests/e2e/test-ucc-lifecycle.sh | sed -n '17,23p;46,52p;64,70p;157,172p;189,195p;218,218p'Repository: kundeng/splunk-devcontainer-template
Length of output: 1962
🏁 Script executed:
cat -n tests/e2e/helpers.shRepository: kundeng/splunk-devcontainer-template
Length of output: 3007
🏁 Script executed:
grep -A 10 "^fail\|^pass\|^results" tests/e2e/helpers.shRepository: kundeng/splunk-devcontainer-template
Length of output: 453
Capture command exit status in the if condition to avoid set -e bypassing error handling.
With set -euo pipefail at line 6, if task commands return non-zero (indicating failure), the script exits immediately before reaching your if statement. This bypasses the fail call and prevents the final results summary from running.
Use if OUTPUT=$(task ...); then ... else ... fi to capture both the output and exit status, ensuring error conditions trigger fail() bookkeeping and allow results() to execute.
This applies to lines 17, 46, 64, 157, 166, 189, and the check_app_rest call at 218.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test-ucc-lifecycle.sh` around lines 17 - 23, The current checks
capture command output into OUTPUT after running task but do not capture the
exit status, which under set -euo pipefail can cause the script to exit before
fail()/results() run; change each pattern that does OUTPUT=$(task ...) followed
by if echo "$OUTPUT" | grep -q ... to the form that captures both output and
exit status by performing the command inside the if (e.g., if OUTPUT=$(task
ucc:init APP_NAME="${TEST_UCC_APP}" 2>&1); then ... else fail ...; echo "
Output: ${OUTPUT:0:300}" fi) for all occurrences mentioned (the ucc:init check
using OUTPUT, the other task invocations at the other noted locations, and the
check_app_rest invocation) so fail() bookkeeping and results() still run on
command failures.
Comment on lines
+148
to
+153
| # Verify OpenAPI spec | ||
| if [ -f "${BUILD_DIR}/appserver/static/openapi.json" ]; then | ||
| pass "OpenAPI spec generated" | ||
| else | ||
| fail "OpenAPI spec missing (non-critical)" | ||
| fi |
There was a problem hiding this comment.
Pick one severity for the OpenAPI check.
If openapi.json is genuinely non-critical, this should be a warning/skip instead of fail; otherwise drop the “non-critical” wording. Right now the suite fails on an artifact it labels optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test-ucc-lifecycle.sh` around lines 148 - 153, Decide whether
OpenAPI generation is required or optional, then update the check accordingly:
if it's optional, replace the fail call in the openapi.json existence block with
a non-failing warning/skip (e.g., call warn/skip or echo a warning) and keep the
"non-critical" wording; if it's required, keep the fail behavior but remove the
"(non-critical)" text from the message so the failure's severity is consistent;
locate the check that tests for "${BUILD_DIR}/appserver/static/openapi.json" and
adjust the pass/fail call there.
On native Linux, Splunk (UID 41812) owns bind-mounted dirs and the devcontainer user (UID 1000) can't write to them. This broke app:create and other tasks that mkdir inside splunk/config/apps/. Fix: app:sync-links now applies POSIX ACLs (setfacl -R -m u:1000:rwX) on dev-apps/, react/, and ucc-apps/ inside the Splunk container. Default ACLs ensure new files inherit access. On Mac/OrbStack this is a harmless no-op since VirtioFS handles permissions transparently. Also adds splunkbase-download.py with credential fallback chain (.env → env vars → built-in account) and research/planning memory files for REST connector architecture decisions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
splunk/scripts/splunkbase-download.py (1)
36-49: Consider handling quoted values in .env parsing.The parser doesn't strip surrounding quotes from values, so
KEY="value"yields"value"(with quotes) rather thanvalue. This is common in.envfiles.♻️ Optional enhancement
if "=" in line: key, _, val = line.partition("=") - env[key.strip()] = val.strip() + val = val.strip() + # Strip surrounding quotes if present + if len(val) >= 2 and val[0] == val[-1] and val[0] in ('"', "'"): + val = val[1:-1] + env[key.strip()] = val return env🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@splunk/scripts/splunkbase-download.py` around lines 36 - 49, The load_env function currently returns values with surrounding quotes intact; update load_env to detect and strip matching surrounding single or double quotes from the parsed val (from the key/val partition) and also unescape common escaped characters inside the quoted string (e.g., \" or \'), before storing it in the env dict; ensure this logic is applied where env[key.strip()] = val.strip() so both KEY="value" and KEY='value' become value and empty or unmatched quotes are handled safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/memory/research_checkpointing_and_dpath.md:
- Around line 402-405: The fenced code block showing the requirements snippet
lacks a language tag (triggers markdownlint MD040); update the fence that opens
before the "package/lib/requirements.txt" snippet to include a language
identifier such as "txt" (e.g., change "```" to "```txt") so the block is
properly tagged and linting passes; locate the block in the file
.claude/memory/research_checkpointing_and_dpath.md by searching for the lines
containing "package/lib/requirements.txt" and the surrounding triple-backticks
and add the language tag.
In @.claude/memory/research_cimplicity_app.md:
- Around line 13-18: The fenced code block shown lacks a language identifier;
update the opening backticks so the block is marked as bash (i.e., change ``` to
```bash) to enable proper syntax highlighting and satisfy markdownlint rules for
the block containing the ucc-gen / cp / yarn build commands.
In `@splunk/scripts/splunkbase-download.py`:
- Around line 93-118: The download_from_splunkbase function can leave a partial
output_path file on disk when an error occurs mid-download; modify
download_from_splunkbase so that on any exception (both urllib.error.HTTPError
and the generic Exception) it closes any open file and removes the partially
written file (check os.path.exists(output_path) and os.remove(output_path)),
then re-use the existing error logging and return False; ensure you only delete
the file on failure (i.e., not after successful write) and reference the symbols
download_from_splunkbase, output_path, and the existing except blocks for where
to add the cleanup.
- Around line 29-32: Remove the hardcoded plaintext fallbacks FALLBACK_USERNAME
and FALLBACK_PASSWORD (leave SPLUNKBASE_LOGIN_URL unchanged) and update the
credential-loading logic in resolve_credentials to not silently use defaults;
instead, make resolve_credentials raise/return an explicit error or exit when no
credentials are provided via environment/secure store so authentication is
required. Ensure references to FALLBACK_USERNAME/FALLBACK_PASSWORD are deleted
or replaced with calls to resolve_credentials so no plaintext credentials remain
in the code or repo.
---
Nitpick comments:
In `@splunk/scripts/splunkbase-download.py`:
- Around line 36-49: The load_env function currently returns values with
surrounding quotes intact; update load_env to detect and strip matching
surrounding single or double quotes from the parsed val (from the key/val
partition) and also unescape common escaped characters inside the quoted string
(e.g., \" or \'), before storing it in the env dict; ensure this logic is
applied where env[key.strip()] = val.strip() so both KEY="value" and KEY='value'
become value and empty or unmatched quotes are handled safely.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2acd5d2a-b0a0-4891-b122-00c94c425048
📒 Files selected for processing (11)
.claude/memory/MEMORY.md.claude/memory/feedback_git_workflow.md.claude/memory/project_rest_connector_plan.md.claude/memory/research_airbyte_cdk_standalone.md.claude/memory/research_airbyte_declarative.md.claude/memory/research_checkpointing_and_dpath.md.claude/memory/research_cimplicity_app.md.claude/memory/research_orbstack_permissions.md.claude/memory/research_ucc_custom_ui.mdTaskfile.ymlsplunk/scripts/splunkbase-download.py
✅ Files skipped from review due to trivial changes (5)
- .claude/memory/feedback_git_workflow.md
- .claude/memory/research_ucc_custom_ui.md
- .claude/memory/MEMORY.md
- .claude/memory/research_airbyte_cdk_standalone.md
- .claude/memory/project_rest_connector_plan.md
Comment on lines
+402
to
+405
| ``` | ||
| # package/lib/requirements.txt | ||
| dpath>=2.1.0,<3.0.0 | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced code block (markdownlint MD040).
Line 402 opens a fence without a language identifier, which triggers MD040 and can fail markdown lint gates.
💡 Proposed fix
-```
+```txt
# package/lib/requirements.txt
dpath>=2.1.0,<3.0.0</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 402-402: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/memory/research_checkpointing_and_dpath.md around lines 402 - 405,
The fenced code block showing the requirements snippet lacks a language tag
(triggers markdownlint MD040); update the fence that opens before the
"package/lib/requirements.txt" snippet to include a language identifier such as
"txt" (e.g., change "```" to "```txt") so the block is properly tagged and
linting passes; locate the block in the file
.claude/memory/research_checkpointing_and_dpath.md by searching for the lines
containing "package/lib/requirements.txt" and the surrounding triple-backticks
and add the language tag.
Comment on lines
+13
to
+18
| ``` | ||
| 1. ucc-gen build --source ucc-app -o build/ | ||
| 2. cp -R build/cim-plicity/* packages/cim-plicity/src/main/resources/splunk/ | ||
| 3. yarn run build (webpack via lerna) | ||
| 4. ucc-gen package --path cim-plicity/ -o ../dist/ | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to the fenced code block.
The code block should specify a language for proper syntax highlighting and markdown linting compliance. As per coding guidelines, markdownlint-cli2 flagged this as missing a language specification.
📝 Proposed fix
-```
+```bash
1. ucc-gen build --source ucc-app -o build/
2. cp -R build/cim-plicity/* packages/cim-plicity/src/main/resources/splunk/
3. yarn run build (webpack via lerna)
4. ucc-gen package --path cim-plicity/ -o ../dist/</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @.claude/memory/research_cimplicity_app.md around lines 13 - 18, The fenced
code block shown lacks a language identifier; update the opening backticks so
the block is marked as bash (i.e., change tobash) to enable proper
syntax highlighting and satisfy markdownlint rules for the block containing the
ucc-gen / cp / yarn build commands.
</details>
<!-- fingerprinting:phantom:triton:puma -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+29
to
+32
| SPLUNKBASE_LOGIN_URL = "https://splunkbase.splunk.com/api/account:login/" | ||
| FALLBACK_USERNAME = "bayeslearner@outlook.com" | ||
| FALLBACK_PASSWORD = "Welcome1!" | ||
|
|
There was a problem hiding this comment.
Critical: Remove hardcoded credentials from source code.
Committing plaintext credentials to version control is a serious security violation:
- Credentials are exposed to anyone with repository access (including forks, CI logs, etc.)
- They persist in git history even after removal
- Silent fallback to these credentials masks misconfiguration
Remove the fallback credentials entirely and require explicit authentication.
🔒 Proposed fix
SPLUNKBASE_LOGIN_URL = "https://splunkbase.splunk.com/api/account:login/"
-FALLBACK_USERNAME = "bayeslearner@outlook.com"
-FALLBACK_PASSWORD = "Welcome1!"Also update resolve_credentials to fail explicitly when credentials are missing:
def resolve_credentials(args, env: dict) -> tuple:
"""Resolve Splunkbase credentials from CLI args, .env, environ, or fallback."""
username = (
getattr(args, "username", None)
or env.get("SPLUNKBASE_USERNAME")
or os.environ.get("SPLUNKBASE_USERNAME")
- or FALLBACK_USERNAME
)
password = (
getattr(args, "password", None)
or env.get("SPLUNKBASE_PASSWORD")
or os.environ.get("SPLUNKBASE_PASSWORD")
- or FALLBACK_PASSWORD
)
return username, password📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| SPLUNKBASE_LOGIN_URL = "https://splunkbase.splunk.com/api/account:login/" | |
| FALLBACK_USERNAME = "bayeslearner@outlook.com" | |
| FALLBACK_PASSWORD = "Welcome1!" | |
| SPLUNKBASE_LOGIN_URL = "https://splunkbase.splunk.com/api/account:login/" | |
🧰 Tools
🪛 Ruff (0.15.7)
[error] 31-31: Possible hardcoded password assigned to: "FALLBACK_PASSWORD"
(S105)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@splunk/scripts/splunkbase-download.py` around lines 29 - 32, Remove the
hardcoded plaintext fallbacks FALLBACK_USERNAME and FALLBACK_PASSWORD (leave
SPLUNKBASE_LOGIN_URL unchanged) and update the credential-loading logic in
resolve_credentials to not silently use defaults; instead, make
resolve_credentials raise/return an explicit error or exit when no credentials
are provided via environment/secure store so authentication is required. Ensure
references to FALLBACK_USERNAME/FALLBACK_PASSWORD are deleted or replaced with
calls to resolve_credentials so no plaintext credentials remain in the code or
repo.
Comment on lines
+93
to
+118
| def download_from_splunkbase(auth: SplunkbaseAuth, app_id: str, | ||
| version: str, output_path: str) -> bool: | ||
| """Download an app from Splunkbase to a local file.""" | ||
| sb_url = f"https://splunkbase.splunk.com/app/{app_id}/release/{version}/download" | ||
| print(f" Downloading from: {sb_url}") | ||
|
|
||
| req = urllib.request.Request(sb_url) | ||
| req.add_header("X-Auth-Token", auth.token) | ||
|
|
||
| try: | ||
| with urllib.request.urlopen(req, timeout=120) as resp: | ||
| with open(output_path, "wb") as f: | ||
| while True: | ||
| chunk = resp.read(8192) | ||
| if not chunk: | ||
| break | ||
| f.write(chunk) | ||
| size_mb = os.path.getsize(output_path) / (1024 * 1024) | ||
| print(f" Downloaded: {output_path} ({size_mb:.1f} MB)") | ||
| return True | ||
| except urllib.error.HTTPError as e: | ||
| print(f" ERROR: HTTP {e.code} — {e.reason}") | ||
| return False | ||
| except Exception as e: | ||
| print(f" ERROR: download failed: {e}") | ||
| return False |
There was a problem hiding this comment.
Clean up partial file on download failure.
If the download fails mid-stream (network timeout, HTTP error after body starts), a partial/corrupted .tgz file remains on disk. This could cause confusing failures if the file is later used.
🛡️ Proposed fix
def download_from_splunkbase(auth: SplunkbaseAuth, app_id: str,
version: str, output_path: str) -> bool:
"""Download an app from Splunkbase to a local file."""
sb_url = f"https://splunkbase.splunk.com/app/{app_id}/release/{version}/download"
print(f" Downloading from: {sb_url}")
req = urllib.request.Request(sb_url)
req.add_header("X-Auth-Token", auth.token)
try:
with urllib.request.urlopen(req, timeout=120) as resp:
with open(output_path, "wb") as f:
while True:
chunk = resp.read(8192)
if not chunk:
break
f.write(chunk)
size_mb = os.path.getsize(output_path) / (1024 * 1024)
print(f" Downloaded: {output_path} ({size_mb:.1f} MB)")
return True
except urllib.error.HTTPError as e:
print(f" ERROR: HTTP {e.code} — {e.reason}")
+ if os.path.exists(output_path):
+ os.remove(output_path)
return False
except Exception as e:
print(f" ERROR: download failed: {e}")
+ if os.path.exists(output_path):
+ os.remove(output_path)
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def download_from_splunkbase(auth: SplunkbaseAuth, app_id: str, | |
| version: str, output_path: str) -> bool: | |
| """Download an app from Splunkbase to a local file.""" | |
| sb_url = f"https://splunkbase.splunk.com/app/{app_id}/release/{version}/download" | |
| print(f" Downloading from: {sb_url}") | |
| req = urllib.request.Request(sb_url) | |
| req.add_header("X-Auth-Token", auth.token) | |
| try: | |
| with urllib.request.urlopen(req, timeout=120) as resp: | |
| with open(output_path, "wb") as f: | |
| while True: | |
| chunk = resp.read(8192) | |
| if not chunk: | |
| break | |
| f.write(chunk) | |
| size_mb = os.path.getsize(output_path) / (1024 * 1024) | |
| print(f" Downloaded: {output_path} ({size_mb:.1f} MB)") | |
| return True | |
| except urllib.error.HTTPError as e: | |
| print(f" ERROR: HTTP {e.code} — {e.reason}") | |
| return False | |
| except Exception as e: | |
| print(f" ERROR: download failed: {e}") | |
| return False | |
| def download_from_splunkbase(auth: SplunkbaseAuth, app_id: str, | |
| version: str, output_path: str) -> bool: | |
| """Download an app from Splunkbase to a local file.""" | |
| sb_url = f"https://splunkbase.splunk.com/app/{app_id}/release/{version}/download" | |
| print(f" Downloading from: {sb_url}") | |
| req = urllib.request.Request(sb_url) | |
| req.add_header("X-Auth-Token", auth.token) | |
| try: | |
| with urllib.request.urlopen(req, timeout=120) as resp: | |
| with open(output_path, "wb") as f: | |
| while True: | |
| chunk = resp.read(8192) | |
| if not chunk: | |
| break | |
| f.write(chunk) | |
| size_mb = os.path.getsize(output_path) / (1024 * 1024) | |
| print(f" Downloaded: {output_path} ({size_mb:.1f} MB)") | |
| return True | |
| except urllib.error.HTTPError as e: | |
| print(f" ERROR: HTTP {e.code} — {e.reason}") | |
| if os.path.exists(output_path): | |
| os.remove(output_path) | |
| return False | |
| except Exception as e: | |
| print(f" ERROR: download failed: {e}") | |
| if os.path.exists(output_path): | |
| os.remove(output_path) | |
| return False |
🧰 Tools
🪛 Ruff (0.15.7)
[error] 99-99: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 103-103: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[warning] 116-116: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@splunk/scripts/splunkbase-download.py` around lines 93 - 118, The
download_from_splunkbase function can leave a partial output_path file on disk
when an error occurs mid-download; modify download_from_splunkbase so that on
any exception (both urllib.error.HTTPError and the generic Exception) it closes
any open file and removes the partially written file (check
os.path.exists(output_path) and os.remove(output_path)), then re-use the
existing error logging and return False; ensure you only delete the file on
failure (i.e., not after successful write) and reference the symbols
download_from_splunkbase, output_path, and the existing except blocks for where
to add the cleanup.
…ownload-only mode Add --download-only flag to deps-install.py that supports both ad-hoc downloads (--app-id + --version) and batch downloads from deps.yml. Adds credential fallback chain (CLI args > .env > env vars > built-in). Remove standalone splunkbase-download.py as deps-install.py now subsumes it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
splunk/scripts/deps-install.py (1)
459-461: Consider a non-argv password override.
--passwordwill show up in shell history and process listings. A prompt or--password-stdinkeeps the override without exposing the secret inargv.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@splunk/scripts/deps-install.py` around lines 459 - 461, Change the password handling to avoid exposing secrets in argv: keep the existing parser.add_argument("--username", ...) but replace or augment parser.add_argument("--password", ...) with support for a --password-stdin flag and interactive prompt fallback. Specifically, add a mutually-exclusive option for --password-stdin and remove direct --password from appearing in process listings (or at least deprecate it), then when parsing args if --password-stdin is set read the password from stdin (sys.stdin.read()/readline) and if no stdin and no --password-stdin, prompt securely with getpass.getpass() to obtain the password into the same password variable; ensure you do not echo or log the password and that code consuming password uses that variable (the one currently supplied by parser.add_argument("--password")).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@splunk/scripts/deps-install.py`:
- Around line 21-25: The code currently includes and uses a built-in Splunkbase
fallback account; remove that constant and any references so
resolve_credentials() no longer returns or silently uses a default credential,
and instead returns only CLI args, .env values, or environment variables (or
raises/returns None to signal missing creds). Update resolve_credentials() to
validate presence of username/password and, on missing credentials, surface an
explicit error/exit rather than falling back. Also update the usage/help text to
remove the fallback mention so it lists only CLI, .env, and environment as
credential sources, and remove any tests or comments referring to the built-in
account (references: resolve_credentials(), any
FALLBACK_USERNAME/FALLBACK_PASSWORD symbols, and the script usage/help string).
- Around line 368-383: The run_install function calls find_container(...) but
never checks its return; if no container is running the script proceeds and
later fails with confusing "empty response" errors—update run_install to check
the container value immediately after container =
find_container(args.compose_file, args.env_file, args.service) and fail fast:
print/log a clear error (e.g., "No running container found for service X") and
return a non-zero exit code, or raise a RuntimeError, so subsequent steps
(os.makedirs, downloads, SplunkbaseAuth) do not run when container is None.
- Around line 297-305: The code currently calls resolve_credentials and
sb_auth.login() at the top of run_download_only(), causing unnecessary
Splunkbase authentication even for URL-only batches; modify run_download_only to
first inspect the requested downloads (e.g., check args.app_id and parse
deps.yml for Splunkbase entries) and only call resolve_credentials(...) and
create SplunkbaseAuth(username=..., password=...) followed by sb_auth.login() if
a Splunkbase download is actually required; keep the existing
os.makedirs(args.output_dir, exist_ok=True) behavior and ensure the function
returns early with the same error code if login fails when it is required.
- Around line 332-347: The loop handling deps should treat both url and name as
untrusted: restrict acceptable URL schemes to only http and https before calling
urllib.request.urlretrieve (or better, replace urlretrieve with the existing
streaming helper download_from_splunkbase()), sanitize the target filename using
os.path.basename(name) before joining with args.output_dir to prevent path
traversal, add timeout/streaming behavior (reuse download_from_splunkbase()
which already implements streaming/timeouts) instead of raw urlretrieve to avoid
uncontrolled schemes and blocking, and refactor the duplicated
download/validation logic so both URL-based downloads and the block duplicated
elsewhere reuse a single helper function (e.g., consolidate into the
download_from_splunkbase/download helper) to prevent divergence.
---
Nitpick comments:
In `@splunk/scripts/deps-install.py`:
- Around line 459-461: Change the password handling to avoid exposing secrets in
argv: keep the existing parser.add_argument("--username", ...) but replace or
augment parser.add_argument("--password", ...) with support for a
--password-stdin flag and interactive prompt fallback. Specifically, add a
mutually-exclusive option for --password-stdin and remove direct --password from
appearing in process listings (or at least deprecate it), then when parsing args
if --password-stdin is set read the password from stdin
(sys.stdin.read()/readline) and if no stdin and no --password-stdin, prompt
securely with getpass.getpass() to obtain the password into the same password
variable; ensure you do not echo or log the password and that code consuming
password uses that variable (the one currently supplied by
parser.add_argument("--password")).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 992afb18-6d00-4026-bd74-fc27071ec796
📒 Files selected for processing (1)
splunk/scripts/deps-install.py
Comment on lines
+21
to
+25
| Credentials resolved in order: | ||
| 1. --username / --password CLI args | ||
| 2. SPLUNKBASE_USERNAME / SPLUNKBASE_PASSWORD in .env file | ||
| 3. SPLUNKBASE_USERNAME / SPLUNKBASE_PASSWORD environment variables | ||
| 4. Built-in fallback account |
There was a problem hiding this comment.
Remove the built-in Splunkbase fallback account.
These lines commit a fallback username/password and wire resolve_credentials() to silently use it whenever the caller omits auth. That is both a secret leak and a shared-account backdoor for every install/download path. Please remove the fallback entirely and update the usage text so credentials only come from CLI, .env, or the environment.
Also applies to: 52-53, 74-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@splunk/scripts/deps-install.py` around lines 21 - 25, The code currently
includes and uses a built-in Splunkbase fallback account; remove that constant
and any references so resolve_credentials() no longer returns or silently uses a
default credential, and instead returns only CLI args, .env values, or
environment variables (or raises/returns None to signal missing creds). Update
resolve_credentials() to validate presence of username/password and, on missing
credentials, surface an explicit error/exit rather than falling back. Also
update the usage/help text to remove the fallback mention so it lists only CLI,
.env, and environment as credential sources, and remove any tests or comments
referring to the built-in account (references: resolve_credentials(), any
FALLBACK_USERNAME/FALLBACK_PASSWORD symbols, and the script usage/help string).
Comment on lines
+297
to
+305
| def run_download_only(args, env: dict) -> int: | ||
| """Download app tarballs without installing (no container needed).""" | ||
| username, password = resolve_credentials(args, env) | ||
| sb_auth = SplunkbaseAuth(username=username, password=password) | ||
|
|
||
| if not sb_auth.login(): | ||
| return 1 | ||
|
|
||
| os.makedirs(args.output_dir, exist_ok=True) |
There was a problem hiding this comment.
Delay Splunkbase auth until a Splunkbase download is actually requested.
run_download_only() authenticates before it has inspected args.app_id or deps.yml, so URL-only batches now depend on Splunkbase credentials/network reachability even though they never touch Splunkbase.
🛠️ Suggested change
- if not sb_auth.login():
- return 1
-
os.makedirs(args.output_dir, exist_ok=True)
success = 0
errors = 0
# Ad-hoc download: --app-id + --version
if args.app_id:
versions = args.version or []
if not versions:
print("ERROR: --version required with --app-id")
return 1
+ if not sb_auth.login():
+ return 1
for version in versions:
print(f"\nDownloading app {args.app_id} v{version}...")
output_path = os.path.join(
args.output_dir, f"app-{args.app_id}-v{version}.tgz"
)
@@
elif sb_id and version:
+ if not sb_auth.token and not sb_auth.login():
+ errors += 1
+ continue
output_path = os.path.join(
args.output_dir, f"app-{sb_id}-v{version}.tgz"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@splunk/scripts/deps-install.py` around lines 297 - 305, The code currently
calls resolve_credentials and sb_auth.login() at the top of run_download_only(),
causing unnecessary Splunkbase authentication even for URL-only batches; modify
run_download_only to first inspect the requested downloads (e.g., check
args.app_id and parse deps.yml for Splunkbase entries) and only call
resolve_credentials(...) and create SplunkbaseAuth(username=..., password=...)
followed by sb_auth.login() if a Splunkbase download is actually required; keep
the existing os.makedirs(args.output_dir, exist_ok=True) behavior and ensure the
function returns early with the same error code if login fails when it is
required.
Comment on lines
+332
to
+347
| for dep in deps: | ||
| name = dep["name"] | ||
| sb_id = dep["splunkbase_id"] | ||
| version = dep["version"] | ||
| url = dep["url"] | ||
|
|
||
| if url: | ||
| output_path = os.path.join(args.output_dir, f"{name}.tgz") | ||
| print(f"\nDownloading {name} from URL: {url}") | ||
| try: | ||
| urllib.request.urlretrieve(url, output_path) | ||
| size_mb = os.path.getsize(output_path) / (1024 * 1024) | ||
| print(f" Downloaded: {output_path} ({size_mb:.1f} MB)") | ||
| success += 1 | ||
| except Exception as e: | ||
| print(f" ERROR: download failed: {e}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the structure and find the file
find . -name "deps-install.py" -type fRepository: kundeng/splunk-devcontainer-template
Length of output: 112
🏁 Script executed:
# Get the relevant code sections
cat -n splunk/scripts/deps-install.py | sed -n '330,350p'Repository: kundeng/splunk-devcontainer-template
Length of output: 1132
🏁 Script executed:
# Check the second location mentioned (lines 402-410)
cat -n splunk/scripts/deps-install.py | sed -n '400,415p'Repository: kundeng/splunk-devcontainer-template
Length of output: 783
🏁 Script executed:
# Search for the download_from_splunkbase function
rg -n "def download_from_splunkbase" splunk/scripts/deps-install.pyRepository: kundeng/splunk-devcontainer-template
Length of output: 147
🏁 Script executed:
# Get the full function to understand the streaming helper
rg -n "def download_from_splunkbase" -A 30 splunk/scripts/deps-install.pyRepository: kundeng/splunk-devcontainer-template
Length of output: 1409
Treat deps.yml URL entries and filenames as untrusted input.
Both url and name are used here without validation. With a user-supplied --deps-file, urlretrieve() will accept non-HTTP schemes (e.g., file://), and a malicious name like ../../../etc/passwd will escape output_dir via os.path.join() behavior. Additionally, urlretrieve() lacks timeout protection and streaming, unlike the download_from_splunkbase() helper.
Please:
- Whitelist URL schemes to
https/httponly - Sanitize filenames with
os.path.basename() - Add timeout handling (or reuse the streaming helper from
download_from_splunkbase()) - Consolidate the duplicate pattern at lines 402-410 to avoid divergence
🧰 Tools
🪛 Ruff (0.15.7)
[error] 342-342: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[warning] 346-346: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@splunk/scripts/deps-install.py` around lines 332 - 347, The loop handling
deps should treat both url and name as untrusted: restrict acceptable URL
schemes to only http and https before calling urllib.request.urlretrieve (or
better, replace urlretrieve with the existing streaming helper
download_from_splunkbase()), sanitize the target filename using
os.path.basename(name) before joining with args.output_dir to prevent path
traversal, add timeout/streaming behavior (reuse download_from_splunkbase()
which already implements streaming/timeouts) instead of raw urlretrieve to avoid
uncontrolled schemes and blocking, and refactor the duplicated
download/validation logic so both URL-based downloads and the block duplicated
elsewhere reuse a single helper function (e.g., consolidate into the
download_from_splunkbase/download helper) to prevent divergence.
Comment on lines
+368
to
383
| def run_install(args, env: dict) -> int: | ||
| """Download and install deps into a running Splunk container.""" | ||
| os.environ.update(env) | ||
| password = env.get("SPLUNK_PASSWORD", os.environ.get("SPLUNK_PASSWORD", "")) | ||
|
|
||
| # Parse deps | ||
| deps = parse_deps(args.deps_file) | ||
| if not deps: | ||
| print("No dependencies to install.") | ||
| return 0 | ||
|
|
||
| # Find container | ||
| container = find_container(args.compose_file, args.env_file, args.service) | ||
| if not container: | ||
| print("ERROR: Splunk container not running. Start with: task dev:up") | ||
| return 1 | ||
|
|
||
| os.makedirs(args.stage_dir, exist_ok=True) | ||
|
|
||
| # Auth (lazy — only if needed) | ||
| sb_auth = SplunkbaseAuth( | ||
| username=env.get("SPLUNKBASE_USERNAME", os.environ.get("SPLUNKBASE_USERNAME", "")), | ||
| password=env.get("SPLUNKBASE_PASSWORD", os.environ.get("SPLUNKBASE_PASSWORD", "")), | ||
| ) | ||
| username, sb_password = resolve_credentials(args, env) | ||
| sb_auth = SplunkbaseAuth(username=username, password=sb_password) | ||
|
|
There was a problem hiding this comment.
Fail fast when install mode has no running container.
This flow never checks whether find_container() returned anything. Direct python3 splunk/scripts/deps-install.py invocations bypass the Taskfile guard in Taskfile.yml Lines 378-384 and then collapse into repeated "empty response" failures. Returning here keeps the error actionable and avoids downloading artifacts that cannot be installed.
🛠️ Suggested change
container = find_container(args.compose_file, args.env_file, args.service)
+ if not container:
+ print(
+ f"ERROR: service '{args.service}' is not running. "
+ "Start it first or use --download-only."
+ )
+ return 1
os.makedirs(args.stage_dir, exist_ok=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@splunk/scripts/deps-install.py` around lines 368 - 383, The run_install
function calls find_container(...) but never checks its return; if no container
is running the script proceeds and later fails with confusing "empty response"
errors—update run_install to check the container value immediately after
container = find_container(args.compose_file, args.env_file, args.service) and
fail fast: print/log a clear error (e.g., "No running container found for
service X") and return a non-zero exit code, or raise a RuntimeError, so
subsequent steps (os.makedirs, downloads, SplunkbaseAuth) do not run when
container is None.
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
1 participant
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.
Summary
ucc:init,ucc:build,ucc:link,ucc:dev,ucc:packagesplunk install appto run as--user splunkTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
stage:package,stage:up,stage:installtest:lifecyclecommandChores