Skip to content

feat(ai-harness): run upstream provisioner scripts during agents sync [draft] - #519

Closed
Bonco-lab wants to merge 1 commit into
masterfrom
feat/515-run-sf-agents-harness-provisioner
Closed

Bonco-lab wants to merge 1 commit into
masterfrom
feat/515-run-sf-agents-harness-provisioner

Conversation

@Bonco-lab

@Bonco-lab Bonco-lab commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Refs: #515
Assisted-by: claude-code/claude-opus-4-8

related to sparkfabrik/sf-agents-harness#98
it must be merged after that one has been merged

Refs: #515
Assisted-by: claude-code/claude-opus-4-8
Copilot AI review requested due to automatic review settings June 11, 2026 14:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds support for running executable “provisioner” scripts as part of the agents harness workflow, enabling generation of CLI-based integrations that can’t be shipped as static files.

Changes:

  • Execute provisioners/*.sh scripts after the sync file-copy phase (sync verb).
  • Execute the same provisioners in the status command (status verb).
  • Document the new provisioner mechanism in the changelog.

Reviewed changes

Copilot reviewed 1 out of 3 changed files in this pull request and generated no comments.

File Description
bin/sparkdock-agents-sync Adds run_provisioners() and invokes it after print_summary.
bin/sparkdock-agents-status Runs provisioner scripts with status verb when present.
CHANGELOG.md Documents provisioner support and invocation behavior.

@sparkfabrik-ai-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

515 - Partially compliant

Compliant requirements:

  • A provisioner mechanism is added that runs executable *.sh scripts from the upstream cache provisioners/ directory, which enables shipping openspec skills/prompts as global resources via provisioner scripts
  • The provisioner runs automatically after the file-copy phase in sparkdock-agents-sync
  • Status check for provisioners is added in sparkdock-agents-status

Non-compliant requirements:

  • The actual openspec skills and prompt files/provisioner script are not included in this PR (referenced as a related PR in sf-agents-harness)

Requires further human verification:

  • Verify that the related sf-agents-harness PR (Replace dinghy with something modern like Traefik #98) actually ships the openspec provisioner script that this mechanism is designed to run
  • Confirm that the provisioner approach satisfies the use case of making openspec available globally without per-repo initialization
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns

Arbitrary script execution:
The run_provisioners function executes any *.sh file found in the upstream cache provisioners/ directory without any integrity verification (e.g., checksum or signature check). If the cache directory is writable by an attacker or if the upstream source is compromised, malicious scripts could be executed with the user's privileges. This is a supply-chain risk worth documenting or mitigating.

⚡ Recommended focus areas for review

Indentation Inconsistency

The new provisioners status block is not indented consistently with the surrounding code — the PROVISIONERS_DIR variable and the if block start at column 0 while the rest of the file uses consistent indentation. This may be a style issue but could also indicate the block was pasted outside an intended scope.

PROVISIONERS_DIR="${CACHE_DIR}/provisioners"
if [[ -d "${PROVISIONERS_DIR}" ]]; then
    for p in "${PROVISIONERS_DIR}"/*.sh; do
        [[ -x "${p}" ]] || continue
        "${p}" status || true
    done
fi
Glob No-match Behavior

If provisioners/ exists but contains no *.sh files, the glob "${provisioners_dir}"/*.sh will expand to a literal string (when nullglob is not set), causing the loop to attempt to execute a non-existent file. A [[ -f "${p}" ]] guard or enabling nullglob locally should be added to handle the empty-directory case safely.

for p in "${provisioners_dir}"/*.sh; do
    [[ -x "${p}" ]] || continue
    log_info "Running provisioner: $(basename "${p}") ${verb}"
    # Never let one provisioner abort the whole sync (set -e).
    "${p}" "${verb}" || log_warn "Provisioner failed: $(basename "${p}")"
done

@sparkfabrik-ai-bot

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against empty glob expansion

When no .sh files exist in the directory, the glob .sh expands to the literal
string "${PROVISIONERS_DIR}/
.sh", causing the script to attempt to execute a
non-existent file. Add a guard to check that the glob matched at least one real file
before iterating. This is the same pattern that should also be applied in
run_provisioners in sparkdock-agents-sync.

bin/sparkdock-agents-status [415-418]

 for p in "${PROVISIONERS_DIR}"/*.sh; do
+    [[ -f "${p}" ]] || continue
     [[ -x "${p}" ]] || continue
     "${p}" status || true
 done
Suggestion importance[1-10]: 6

__

Why: When no .sh files exist in the directory, the glob expands to a literal string. Adding [[ -f "${p}" ]] || continue before the executable check correctly handles this edge case. This is a valid defensive pattern for bash glob iteration.

Low
General
Validate provisioner verb argument

The run_provisioners function accepts any arbitrary string as verb without
validation. Passing an unexpected or empty verb to a provisioner script could cause
unintended behavior. Add an explicit check that verb is one of the expected values
(sync or status) before executing any provisioner.

bin/sparkdock-agents-sync [620-623]

 run_provisioners() {
     local verb="$1"
+    if [[ "${verb}" != "sync" && "${verb}" != "status" ]]; then
+        log_warn "run_provisioners: unknown verb '${verb}', skipping"
+        return 1
+    fi
     local provisioners_dir="${CACHE_DIR}/provisioners"
     [[ -d "${provisioners_dir}" ]] || return 0
 ...
Suggestion importance[1-10]: 3

__

Why: The function run_provisioners is only called internally with hardcoded values (sync), so the risk of an invalid verb is low. While defensive validation is good practice, the impact here is minimal given the controlled call sites.

Low

@Bonco-lab

Copy link
Copy Markdown
Contributor Author

We decided to avoid running provisioners scripts from sf-harness repo due to potential security vulnerabilities

@Bonco-lab Bonco-lab closed this Jun 17, 2026
@paolomainardi
paolomainardi deleted the feat/515-run-sf-agents-harness-provisioner branch June 17, 2026 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants