Switch to bubblewrap & many other changes #1
Conversation
|
LGTM will merge later today |
|
I abandoned this pr 💀 |
|
uhh what |
|
is this dead?? |
|
Well the project is kinda dead but I'll merge it when I come back here (hopefully soon) |
|
hmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm |
There was a problem hiding this comment.
Pull Request Overview
This PR switches the sandboxing mechanism to use Bubblewrap, scaffolds Python language support, and updates setup and container configurations.
- Introduces
getBwrapPathandcreateBwrapArgshelpers to spawn compile/run under Bubblewrap - Adds non-interactive
--assume-yesflag tosetup.bashand Python language files - Updates Dockerfile and
docker-compose.ymlfor production deployment
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/execute.ts | Added Bubblewrap helper functions, updated spawn calls, and awaited file writes |
| setup.bash | Added --assume-yes/-y flag to skip confirmation prompt |
| languages/python/run.bash | New Python run script |
| languages/python/meta.json | New Python metadata configuration |
| languages/python/compile.sh | New Python compile stub |
| docker-compose.yml | Defined codefort service, network, and security settings |
| Dockerfile | Installed dependencies, copied files, ran setup script, and cleaned up |
Comments suppressed due to low confidence (4)
src/execute.ts:116
- projectRootPath is not defined in this file; ensure you import or define projectRootPath or use a correctly scoped variable for the project root.
);
src/execute.ts:198
- Remove the debug console.log(options) statement or replace it with a proper logging mechanism to avoid leaking sensitive data in production.
console.log(options);
languages/python/meta.json:4
- The field 'runScript' may not match the code's expected 'runPath' property; update the key to 'runPath' or adjust the code to use 'runScript'.
"runScript": "./run.bash",
setup.bash:20
- [nitpick] The negation and regex syntax is unconventional; prefer
if [[ ! "$REPLY" =~ ^[Yy]$ ]]; thenwithout parentheses around the test expression.
if [[ !("$REPLY" =~ ^[Yy]$) ]]; then
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
WalkthroughThis update introduces containerization and sandboxing improvements. It adds a Dockerfile and docker-compose.yml for deployment, scripts and metadata for Python language execution, and enhances the setup script with non-interactive options. The execution logic is refactored to use bubblewrap for sandboxed command execution, replacing the previous isolation method. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant Bubblewrap
participant LanguageScript
User->>API: Submit code for execution
API->>Bubblewrap: Spawn process with bwrap args (sandboxed)
Bubblewrap->>LanguageScript: Run compile/run script (e.g., run.bash)
LanguageScript-->>Bubblewrap: Output/result
Bubblewrap-->>API: Sandboxed output
API-->>User: Return execution result
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
languages/python/meta.json (1)
1-6: Meta looks good – previous naming concern resolved
"name": "Python"matches earlier feedback. 👍docker-compose.yml (1)
12-17: Critical security risk: Avoid disabling security confinement and granting excessive privilegesThe combination of disabling AppArmor/seccomp confinement and granting SYS_ADMIN capability creates significant security vulnerabilities. This configuration essentially gives the container near-root privileges on the host system.
Since bubblewrap provides its own sandboxing, consider:
- Keeping AppArmor and seccomp enabled for defense-in-depth
- Using more granular capabilities instead of SYS_ADMIN
- Running bubblewrap with user namespaces if possible
src/execute.ts (1)
77-77: Add TypeScript annotationsThe function lacks type annotations for its parameters.
-function createBwrapArgs(tempDir, language, command) { +function createBwrapArgs(tempDir: string, language: LanguageType, command: string): string[] {
🧹 Nitpick comments (5)
languages/python/compile.sh (1)
1-2: No-op compile script – confirm necessity or drop it
compile.shexits immediately. If the Python flow never needs a compile step, consider removing this script entirely to avoid confusing contributors (the system will happily fall back to justrun.bash).setup.bash (1)
7-14: Considergetoptsfor flag parsingManual loop works, but
getopts(or a minimal while-loop) would give clearer semantics and automatic help for-h. Low priority.languages/python/run.bash (1)
1-2: Propagate exit codes & signals withexecReplace the subshell with
execso the Python process becomes PID 1 inside Bubblewrap / Docker and receives signals correctly:-python3 code.py +exec python3 code.pydocker-compose.yml (1)
27-30: Document the external network requirementThe service depends on an external network
judge-networkthat must be created before runningdocker-compose up. Consider adding setup instructions or a script to create this network.Add to your README or setup documentation:
docker network create judge-networksrc/execute.ts (1)
198-198: Remove debug console.logThis appears to be debug code that should be removed.
- console.log(options);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Dockerfile(1 hunks)docker-compose.yml(1 hunks)languages/python/compile.sh(1 hunks)languages/python/meta.json(1 hunks)languages/python/run.bash(1 hunks)setup.bash(1 hunks)src/execute.ts(5 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
setup.bash
[error] 20-20: You are missing a required space here.
(SC1035)
🔇 Additional comments (1)
src/execute.ts (1)
152-152: Good fix: Properly awaiting file writeThe addition of
awaitensures the file is written before proceeding with compilation/execution.
| read -r -p "Are you sure you want to proceed? [y/N] " -n 1 | ||
| echo | ||
| if [[ !("$REPLY" =~ ^[Yy]$) ]]; then | ||
| echo "Ending..." | ||
| exit 1 |
There was a problem hiding this comment.
Fix ShellCheck SC1035 – missing space after ! breaks pattern-match test
Current line (20) is parsed incorrectly and triggers SC1035:
if [[ !("$REPLY" =~ ^[Yy]$) ]]; thenCorrect form:
- if [[ !("$REPLY" =~ ^[Yy]$) ]]; then
+ if [[ ! "$REPLY" =~ ^[Yy]$ ]]; thenWithout the space, [[ treats !("$REPLY" as a single token, the test always fails, and the script may exit even when the user answered “y”.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 20-20: You are missing a required space here.
(SC1035)
🤖 Prompt for AI Agents
In setup.bash around lines 18 to 22, the if condition lacks a space after the
negation operator !, causing incorrect parsing and always failing the test. Add
a space after ! so the condition reads if [[ ! ("$REPLY" =~ ^[Yy]$) ]]; then to
correctly negate the pattern match and ensure the script behaves as intended.
|
|
||
| EXPOSE 3000 | ||
|
|
||
| RUN apt-get clean all |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate cleanup layer – remove or merge
Second RUN apt-get clean all repeats the (broken) command and adds another layer. Merge cleanup into the first RUN or delete this line.
🤖 Prompt for AI Agents
In Dockerfile at line 18, the command `RUN apt-get clean all` is duplicated and
creates an unnecessary extra layer. Remove this line or merge the cleanup
commands into the initial `RUN` instruction that performs package installation
and cleanup to optimize the image and avoid redundancy.
| RUN apt-get update && apt-get install -y python3 golang curl tar bubblewrap && apt-get clean all && \ | ||
| find / -type d -name '*cache*' -o -type f \( -name '*.pyc' -o -name '*.pyo' \) -exec rm -rf {} + 2>/dev/null |
There was a problem hiding this comment.
apt-get clean all is invalid – will fail the build
apt-get clean takes no arguments. Use:
-RUN apt-get update && apt-get install -y python3 golang curl tar bubblewrap && apt-get clean all && \
+RUN apt-get update && apt-get install -y python3 golang curl tar bubblewrap && apt-get clean && \Same issue later at line 18.
📝 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.
| RUN apt-get update && apt-get install -y python3 golang curl tar bubblewrap && apt-get clean all && \ | |
| find / -type d -name '*cache*' -o -type f \( -name '*.pyc' -o -name '*.pyo' \) -exec rm -rf {} + 2>/dev/null | |
| RUN apt-get update && apt-get install -y python3 golang curl tar bubblewrap && apt-get clean && \ | |
| find / -type d -name '*cache*' -o -type f \( -name '*.pyc' -o -name '*.pyo' \) -exec rm -rf {} + 2>/dev/null |
🤖 Prompt for AI Agents
In Dockerfile at lines 4-5 and 18, the command uses 'apt-get clean all' which is
invalid and causes build failure. Remove the 'all' argument so the command is
just 'apt-get clean'. Update both occurrences accordingly to fix the issue.
There was a problem hiding this comment.
worked for me
qincai in 🌐 pi5-trixie in ~ via py v3.13.5 🐧 6.12.36-v8-16k+ ⏱
❯ sudo apt-get clean all
qincai in 🌐 pi5-trixie in ~ via py v3.13.5 🐧 6.12.36-v8-16k+ ⏱
❯ echo $?
0
There was a problem hiding this comment.
🧩 Analysis chain
@QinCai-rui Thank you for testing this! Your evidence shows that apt-get clean all does work on your system. Let me investigate the current behavior of this command to provide accurate information.
🌐 Web query:
apt-get clean all command behavior debian ubuntu 2024
💡 Result:
The apt-get clean command is used in Debian-based systems, including Ubuntu, to remove all downloaded package files from the local APT cache, located in /var/cache/apt/archives/. This action frees up disk space by deleting .deb files that were downloaded during package installations or upgrades. It's important to note that this command does not affect installed packages; it only removes the cached package files. (networkworld.com)
Regarding the command apt-get clean all, it behaves identically to apt-get clean. The all argument is ignored, and the command performs the same operation as apt-get clean, removing all files from the APT cache. This behavior has been consistent in Debian and Ubuntu systems, and there is no difference between the two commands. (askubuntu.com)
For more targeted cache management, you can use the apt-get autoclean command. This command removes only the package files that are no longer available for download from the repositories, such as outdated versions that have been replaced by newer ones. This helps keep the cache from growing too large without removing all cached packages. (networkworld.com)
In summary, apt-get clean and apt-get clean all are functionally identical, both clearing the entire APT cache. For more selective cache cleaning, consider using apt-get autoclean.
Citations:
- 1: https://www.networkworld.com/article/968090/cleaning-up-with-apt-get.html?utm_source=openai
- 2: https://askubuntu.com/questions/144222/how-do-apt-get-clean-and-apt-get-clean-all-differ?utm_source=openai
- 3: https://www.networkworld.com/article/968090/cleaning-up-with-apt-get.html?utm_source=openai
@QinCai-rui You're absolutely right! I apologize for the incorrect information. Based on current documentation, apt-get clean all is actually functionally identical to apt-get clean - the all argument is simply ignored by the command, but it doesn't cause an error or build failure.
Your testing demonstrates this perfectly with the exit code 0. The command works as expected and will successfully clean the APT cache in your Dockerfile.
Thank you for taking the time to test this and provide evidence. Your correction helps ensure accurate information for future reviews.
(\ /)
( ._.)
o_(")(")
(learning something new!)
✏️ Learnings added
Learnt from: QinCai-rui
PR: webdev03/codefort#1
File: Dockerfile:4-5
Timestamp: 2025-07-17T06:18:58.758Z
Learning: The command `apt-get clean all` is functionally identical to `apt-get clean` in Debian-based systems. The `all` argument is ignored but does not cause errors or build failures. Both commands successfully clear the APT cache.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@QinCai-rui
from user373986 on AskUbuntu:
The affectation of adding the "all" is most likely a hangover from "yum clean all"
this is what happens when you vibe code
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Docstrings generation was requested by @webdev03. * #1 (comment) The following files were modified: * `src/execute.ts`
|
Note Generated docstrings for this pull request at #2 |
you know
Summary by CodeRabbit
New Features
Refactor