Skip to content

Pass standard target packaging env vars to targets#33

Merged
tomusdrw merged 2 commits intomainfrom
standard-target-packaging-env
May 4, 2026
Merged

Pass standard target packaging env vars to targets#33
tomusdrw merged 2 commits intomainfrom
standard-target-packaging-env

Conversation

@tomusdrw
Copy link
Copy Markdown
Member

@tomusdrw tomusdrw commented May 4, 2026

Summary

Sets the standard target packaging env vars on every target container so teams that have migrated can rely on them, while keeping the existing {TARGET_SOCK} cmd-line substitution as a fallback.

  • Inject JAM_FUZZ=1, JAM_FUZZ_SPEC=tiny, JAM_FUZZ_DATA_PATH=/shared/data, JAM_FUZZ_SOCK_PATH=/shared/jam_target.sock into every startTarget call.
  • User-provided TARGET_ENV / docker_env is appended last so teams can still override (docker -e is last-wins).
  • createSharedVolume now also creates /shared/data (777) so targets can write their cache there.
  • clearDataDir wipes /shared/data between sequential fuzz-source runs to match official testing's fresh-init behavior between sessions. First run starts on an already-fresh volume, so we only clear before runs 2..N.

JAM_FUZZ_LOG_LEVEL is intentionally not set by default — teams already set their own logging via docker_env (e.g. typeberry uses JAM_LOG=log).

Out of scope / not changed

  • Workflow YAMLs (env vars are injected by the harness, no per-team config needed).
  • Picofuzz --repeat sessions: the picofuzz binary drives those Initialize cycles internally, so the harness can't easily clear between them. The data dir is a startup cache per the spec, and per-session correctness is the target's responsibility on Initialize reset.
  • fuzzSource — graymatter is the source, not a target.

Test plan

  • Trigger a *-performance workflow on a representative target and confirm it still passes (legacy targets must keep working via {TARGET_SOCK} fallback).
  • Trigger a *-fuzz workflow with num_runs > 1 and verify the data dir is cleared between runs (look for the alpine rm -rf /shared/data step in logs).
  • Confirm docker inspect of a running target shows the new env vars set.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation

    • Expanded README with setup and configuration for fuzz target containers, clarifying standard environment variables, socket path usage, backward compatibility for existing socket placeholders, env var override behavior, and data-directory initialization/cleanup expectations.
  • Tests

    • Strengthened test harness: standardized target environment handling, ensured user-provided env vars take precedence, initialized shared volumes reliably, and added automated data-directory cleanup between sequential fuzzing runs.

Targets now receive JAM_FUZZ, JAM_FUZZ_SPEC, JAM_FUZZ_DATA_PATH, and
JAM_FUZZ_SOCK_PATH on every startup per the jam-conformance fuzz-proto
spec, while the legacy {TARGET_SOCK} cmd-line substitution stays as a
fallback so existing targets keep working unchanged.

The data dir is wiped between sequential fuzz-source runs to match
official testing's fresh-init behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8ca6adab-2519-462e-8f5d-04dd57615742

📥 Commits

Reviewing files that changed from the base of the PR and between 9dc626a and e419d06.

📒 Files selected for processing (2)
  • README.md
  • tests/common.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • README.md
  • tests/common.ts

📝 Walkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and concisely describes the main change: passing standard target packaging environment variables to container targets, which aligns perfectly with the PR's primary objective.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch standard-target-packaging-env

Review rate limit: 3/5 reviews remaining, refill in 16 minutes and 55 seconds.

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/common.ts`:
- Around line 118-122: The shell command in tests/common.ts currently
interpolates volumeName directly into execSync command strings (see
createSharedVolume() and clearDataDir() where execSync(...) builds `... -v
${volumeName}:/shared ...`), which risks shell injection if volumeName contains
metacharacters; change these to invoke Docker without a single interpolated
shell string by using a child_process API that accepts an argv array (e.g.,
execFileSync or spawnSync) and pass volumeName as a separate argument (or
construct the -v argument as a single array element like
`${volumeName}:/shared`) instead of embedding it in a shell, and apply the same
fix to the other similar execSync usages noted in the file (lines around
175-184).
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 36e0b3f8-c710-4fe4-b626-c4b98513d8c5

📥 Commits

Reviewing files that changed from the base of the PR and between 1062052 and 9dc626a.

📒 Files selected for processing (3)
  • README.md
  • tests/common.ts
  • tests/fuzz-source/common.ts

Comment thread tests/common.ts Outdated
- Add JAM_FUZZ_LOG_LEVEL=debug to the standard target env vars.
- Switch docker invocations from shell-string execSync to argv-based
  execFileSync (via a small docker() helper) so volumeName interpolation
  isn't shell-parsed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tomusdrw tomusdrw merged commit 7792eb6 into main May 4, 2026
18 checks passed
@tomusdrw tomusdrw deleted the standard-target-packaging-env branch May 4, 2026 20:14
tomusdrw added a commit that referenced this pull request May 6, 2026
The graymatter target fails when started with the legacy fuzz-m1-target
cmd-line args. It now reads JAM_FUZZ, JAM_FUZZ_SOCK_PATH, etc. from the
standard target packaging env vars (added in #33), so we can launch it
with no cmd args at all.

- demo-source.yml: docker_cmd is now optional (default "").
- graymatter-fuzz.yml: drop docker_cmd entirely.
- tests/common.ts: TARGET_CMD no longer required; buildCmdArgs("")
  already collapses to no args.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant