fix(phobos): correct copy helper quoting

[ShudongCai]

Summary

Correct the PhobosCopyTool.sh template so its target-directory assignment uses ASCII double quotes instead of Unicode curly quotation marks, and pin the fix with a regression test. Without this, the copy step in every generated Phobos exercise fails. (Audit finding I-002.)

Linked issues

None. I-002 comes from an internal audit report, not a GitHub issue.

1. Problem

Observed under any configuration that generates a Phobos exercise: the script is a static
template resource, so no Java version, build tool, AOP mode or architecture mode changes it.
PhobosCopyTool.sh assigned the target directory with curly quotation marks:

TARGET_DIR=“/var/tmp/opt/core”

Bash does not treat “ (U+201C) and ” (U+201D) as quoting operators, so they stay in the
value. TARGET_DIR resolves to “/var/tmp/opt/core”, and the following
cp -v SpecificExercise.cfg "$TARGET_DIR/" copies into a directory that does not exist and
fails; under set -e that aborts the script.

The fault sits in plugging Ares into the build: the script is copied verbatim into generated
exercises (PhobosCopyFiles.csv to JavaWriter), so it reaches every generated exercise
rather than staying in the template. Ares neither let forbidden code through nor failed a
correct submission; a setup step failed, which breaks a correct exercise before any
submission is judged.

2. Improvement from the user's perspective

An instructor generating a Phobos exercise gets a copy step that works: SpecificExercise.cfg is copied into /var/tmp/opt/core/ as intended, instead of the exercise breaking at the cp into a non-existent “/var/tmp/opt/core” directory.

3. Improvement from the maintainer's perspective

The authoritative template is now pinned by PhobosShellContractTest.copyToolAssignsTargetDirectoryWithAsciiQuotes, which parses the assignment in Java and rejects any reappearance of the curly quotation marks, so this class of typo cannot silently return. The check is deliberately self-contained: it never executes the fixture and never interpolates its path into a shell command (it reads the file, matches the assignment with a regex, and validates syntax by feeding the content to bash -n on stdin).

4. Testing manual

Prerequisites

1. JDK and Maven as for any Ares 2 build, plus a bash on PATH (the test shells out to bash -n). No exercise repository, policy file or echo server is needed: the fixture is the packaged template resource src/main/resources/de/tum/cit/ase/ares/api/templates/phobos/PhobosCopyTool.sh.
2. Check out this branch.

Steps

1. mvn -o test -Dtest=PhobosShellContractTest -f pom.xml
2. Optionally, open PhobosCopyTool.sh and confirm the assignment reads TARGET_DIR="/var/tmp/opt/core".

Expected result

• copyToolAssignsTargetDirectoryWithAsciiQuotes passes: the script contains no “/”, contains exactly one TARGET_DIR= assignment, that assignment matches ^TARGET_DIR="(/var/tmp/opt/core)"$, and bash -n on the script content exits 0.

Negative case (what must still be rejected)

• Reintroducing the curly quotation marks (or changing the value, or adding a second TARGET_DIR= line) makes the test fail: the assertFalse on U+201C/U+201D, the exactly-one-assignment assertEquals(1, ...), and the anchored regex each reject the broken form. The test was confirmed to fail against the original curly-quote script before the fix.

Modes exercised

No mode-specific behaviour changed.

• ArchUnit + AspectJ
• ArchUnit + instrumentation
• WALA + AspectJ
• WALA + instrumentation

5. Test case coverage regarding this PR

No production Java code changed. The corrected artefact is the shell-script template resource PhobosCopyTool.sh; the only Java added is the regression test PhobosShellContractTest, which pins that resource. There is therefore no production class with Instruction/Branch/Line/Method coverage to report.

Breaking changes and migration

None. The public API under de.tum.cit.ase.ares.api, the security policy file format, the generated security test code and the minimum JDK/Maven/Gradle versions are all unchanged. The only behavioural change is that the copy step in a newly generated exercise now succeeds where it previously failed.

Checklist

• The title of this pull request describes the change, not the implementation.
• I followed the guidelines for inclusive, diversity-sensitive and appreciative language.

• Tests were added or updated for the behaviour changed here.

• CI is green, or every remaining failure is explained above.
• No secrets, tokens or absolute local paths are contained in the diff.

Review progress

• Code review
• Manual test

[coderabbitai]

📝 Walkthrough

Walkthrough

The Phobos copy tool now assigns /var/tmp/opt/core using ASCII double quotes. A contract test verifies the assignment, rejects curly quotes, checks Bash syntax, and confirms the resolved directory value.

Changes

Phobos shell contract

Layer / File(s)	Summary
Target directory quoting and validation src/main/resources/.../PhobosCopyTool.sh, src/test/.../PhobosShellContractTest.java	TARGET_DIR uses ASCII double quotes, with tests covering the exact assignment, curly quote rejection, Bash syntax, and resolved value.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: krusche

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
Trusted Boundary Preservation	❌ Error	The new test shells out with interpolated paths and eval on fixture content, so trusted test code now executes shell source across the boundary.	Remove bash -c string construction and eval; parse TARGET_DIR in Java or pass file paths as separate ProcessBuilder arguments only.

✅ Passed checks (7 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Sandbox Fail-Closed Behaviour	✅ Passed	Only a quoting fix in a template and a test were changed; no enforcement logic or fail-closed paths were modified.
Github Workflow Least Privilege	✅ Passed	Diff only changes a shell template and a test; no .github/workflows files were modified, so this least-privilege check is not applicable.
Title check	✅ Passed	The title is concise and accurately describes the Phobos copy-helper automation fix caused by incorrect quoting.
Description check	✅ Passed	The description clearly explains the quoting defect, its automation impact, the regression test, and the scope of the change.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/i-002-phobos-copy-tool-quoting

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/test/java/de/tum/cit/ase/ares/api/phobos/PhobosShellContractTest.java`:
- Around line 61-65: Update PhobosShellContractTest to avoid eval and
shell-source interpolation: parse the TARGET_DIR assignment from copyTool
directly in Java, and invoke Bash through ProcessBuilder with copyTool passed as
an argument rather than embedded in quoted command strings. Preserve the
existing syntax validation and parsed-value assertions while ensuring checkout
paths and fixture contents cannot execute shell syntax.

🪄 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: ASSERTIVE

Plan: Pro Plus

Run ID: edf587b3-032b-408a-a45b-f38dd62315ce

📥 Commits

Reviewing files that changed from the base of the PR and between 6a3c9a6 and ea276da.

📒 Files selected for processing (2)

• src/main/resources/de/tum/cit/ase/ares/api/templates/phobos/PhobosCopyTool.sh
• src/test/java/de/tum/cit/ase/ares/api/phobos/PhobosShellContractTest.java

📜 Review details

⏰ Context from checks skipped due to timeout. (2)

• GitHub Check: Analyse Java
• GitHub Check: Build

🧰 Additional context used

📓 Path-based instructions (3)

**/*

⚙️ CodeRabbit configuration file

Dogmatically check all reviewed files for current British English in prose, comments, JavaDoc, documentation, workflow names, step names, issue/PR text, labels, user-facing messages, and review suggestions. Flag American spellings and grammar such as behavior, color, initialize, authorization, canceled, and program when they are natural-language text. Do not flag programming-language syntax, dependency coordinates, API names, class names, method names, package names, paths, URLs, quoted external identifiers, or other literals where American English is required by the technology.

Files:

• src/main/resources/de/tum/cit/ase/ares/api/templates/phobos/PhobosCopyTool.sh
• src/test/java/de/tum/cit/ase/ares/api/phobos/PhobosShellContractTest.java

**/*Test.java

📄 CodeRabbit inference engine (AGENTS.md)

**/*Test.java: A sandboxed test JVM must never spin up its own server (echo server, socket listener, etc.) to test incoming or outgoing connections
Outgoing-connection tests must connect to an external echo server at a configurable endpoint running as a separate process or CI service on the loopback at port 25565, exercising only the student's client behaviour
If the external echo server is not reachable, the test must skip (using JUnit Assumptions.abort) rather than fail
An Ares SecurityException on an explicitly allowed connection is always a real failure and must propagate (never skipped)
Do not hard-code a self-hosted listener as the connection counterpart; use an external echo service to avoid in-JVM BindException/thread/lifecycle flakiness

Files:

• src/test/java/de/tum/cit/ase/ares/api/phobos/PhobosShellContractTest.java

src/test/java/**/*.java

⚙️ CodeRabbit configuration file

Require tests to distinguish fixture failures from sandbox failures. Network tests must not start in-process listeners inside the sandbox; external fixtures may be skipped when absent, but explicit Ares SecurityException failures must propagate.

Files:

• src/test/java/de/tum/cit/ase/ares/api/phobos/PhobosShellContractTest.java

🔇 Additional comments (1)

src/main/resources/de/tum/cit/ase/ares/api/templates/phobos/PhobosCopyTool.sh (1)

5-5: LGTM!

[MarkusPaulsen]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[Claudia-Anthropica]

@ShudongCai The template now uses ASCII double quotes for the intended target path, and the regression test safely validates the exact assignment and Bash syntax without executing fixture content. The prior shell-boundary concern is resolved, and all captured CI checks succeeded on this head.

[MarkusPaulsen]

The pull request template now bounds each section: 500 characters for Summary, 1000 for
Linked issues, sections 1 to 3 and Breaking changes and migration, 5000 for the testing
manual. The count is what a reader sees, so the template's own instruction comments do not
count towards it.

This description was written before those limits and exceeds 1 of them, so I have
shortened it. Nothing is lost: the original text of every section I touched is kept below,
so you can restore, reword or move any of it yourself.

1. Problem, as it read before (1119 characters, limit 1000)

Observed under any configuration that generates a Phobos exercise (the script is a static template resource, so no Java version, build tool, AOP mode or architecture mode changes the defect). PhobosCopyTool.sh assigned the target directory with Unicode curly quotation marks:

TARGET_DIR=“/var/tmp/opt/core”

Bash does not treat “ (U+201C) and ” (U+201D) as quoting operators, so they stay literal in the value. TARGET_DIR resolves to “/var/tmp/opt/core” rather than /var/tmp/opt/core, and the following cp -v SpecificExercise.cfg "$TARGET_DIR/" then copies into a directory that does not exist and fails (the script also runs under set -e, so the failure aborts it).

The root cause sits in the build-integration layer: this script is copied verbatim into generated exercise artifacts (PhobosCopyFiles.csv -> JavaWriter), so the defect propagates into every generated exercise rather than staying local to the template. It is not a false negative or false positive of the security enforcement itself. It is a setup step that fails closed in the wrong place, breaking a correct exercise.