fix: throwaway repros silently used the live session - #2600
Conversation
kangal-bot
added
the
ai-review
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe throwaway reproduction workflow now starts a headless Herdr server, creates its initial workspace, optionally supports an attached client, targets control commands explicitly, and performs verified cleanup. ChangesThrowaway session isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The change prevents throwaway repros from silently using the live session, but unsafe session-name handling, predictable temporary-file paths, and a session-ownership race can still cause command/path injection, unrelated-file damage, or cleanup of another session. The PR is not merge-ready until these bounded risks are addressed or explicitly accepted. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8731fef1-46d1-4e5a-a8fe-e1a3dc923c35
📒 Files selected for processing (3)
.agents/skills/herdr-throwaway-repro/SKILL.mddocs/next/CHANGELOG.mdskills/herdr/SKILL.md
87141db to
8e86978
Compare
Greptile SummaryThe PR revises the throwaway-reproduction skill to start explicitly named headless sessions and consistently target them with
Confidence Score: 4/5The PR is not yet safe to merge because the documented server startup can still block the driving shell and prevent the throwaway reproduction from proceeding. The skill recognizes that the server is foreground-running but supplies only a direct server invocation and refers to an unspecified background primitive, leaving the previously reported workflow-blocking failure outstanding. Files Needing Attention: .agents/skills/herdr-throwaway-repro/SKILL.md
|
| Filename | Overview |
|---|---|
| .agents/skills/herdr-throwaway-repro/SKILL.md | Reworks disposable-session startup, targeting, client attachment, and cleanup guidance, but the previously reported nonblocking-launch defect remains unresolved. |
Reviews (4): Last reviewed commit: "fix: throwaway repros silently used the ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/skills/herdr-throwaway-repro/SKILL.md (1)
50-60: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRun the server in the background before continuing.
The command ends with
herdr --session <session-name> server, but the server stays in the foreground. A shell or pane that follows these instructions blocks at this step, sosession list, workspace creation, and reproduction commands do not run.Use the installed tool's background primitive, or background the process and redirect its output to a
/var/tmplog.Proposed shell change
- herdr --session <session-name> server + nohup herdr --session <session-name> server \ + >"/var/tmp/<session-name>-server.log" 2>&1 </dev/null &
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7daea657-a646-483a-bf63-c445b1a2ea86
📒 Files selected for processing (2)
.agents/skills/herdr-throwaway-repro/SKILL.mddocs/next/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/next/CHANGELOG.md
c7fc62a to
7708d15
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f247886a-071d-4de4-80a9-a208fdafce70
📒 Files selected for processing (2)
.agents/skills/herdr-throwaway-repro/SKILL.mddocs/next/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/next/CHANGELOG.md
| env \ | ||
| -u HERDR_SOCKET_PATH \ | ||
| -u HERDR_CLIENT_SOCKET_PATH \ | ||
| -u HERDR_SESSION \ | ||
| -u HERDR_WORKSPACE_ID \ | ||
| -u HERDR_TAB_ID \ | ||
| -u HERDR_PANE_ID \ | ||
| herdr --session <session-name> | ||
| herdr --session <session-name> server |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate and quote the session name before shell interpolation.
The suggested repro-<topic>-<timestamp> format does not constrain <topic>. When the placeholder is replaced with whitespace, shell metacharacters, /, or .., the commands can split arguments, execute unintended commands, or escape /var/tmp. Require a name such as ^[A-Za-z0-9][A-Za-z0-9_-]*$, and quote every session-name expansion.
Proposed validation pattern
+case "$session_name" in
+ ''|*[!A-Za-z0-9_-]*) exit 1 ;;
+esac
+
- herdr --session <session-name> server
+ herdr --session "$session_name" server| ```bash | ||
| printf '[experimental]\nallow_nested = true\n' > /var/tmp/<session-name>-config.toml | ||
| ``` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Create the nested-client config with an exclusive temporary path.
printf ... > /var/tmp/<session-name>-config.toml can follow an existing symlink or overwrite an unrelated file. Cleanup can then remove that file. Use mktemp, store the returned path, pass that path through HERDR_CONFIG_PATH, and remove only that exact path.
Proposed temporary-file handling
-printf '[experimental]\nallow_nested = true\n' > /var/tmp/<session-name>-config.toml
+config_path="$(mktemp /var/tmp/herdr-repro-config.XXXXXX)"
+printf '[experimental]\nallow_nested = true\n' >"$config_path"
- HERDR_CONFIG_PATH=/var/tmp/<session-name>-config.toml
+ HERDR_CONFIG_PATH="$config_path"
- Remove /var/tmp/<session-name>-config.toml
+ Remove "$config_path"Also applies to: 166-167
ogulcancelik
left a comment
ogulcancelik
left a comment
There was a problem hiding this comment.
the throwaway-repro correction is useful, but please keep this PR to . tracks the latest stable release and cannot change in normal feature/fix work, and is for user-facing runtime changes rather than project skill maintenance. after dropping those two edits, please rebase onto current master and rerun the checks.
ogulcancelik
left a comment
ogulcancelik
left a comment
There was a problem hiding this comment.
the throwaway-repro correction is useful, but please keep this PR to .agents/skills/herdr-throwaway-repro/SKILL.md. skills/herdr/SKILL.md tracks the latest stable release and cannot change in normal feature/fix work, and docs/next/CHANGELOG.md is for user-facing runtime changes rather than project skill maintenance. after dropping those two edits, please rebase onto current master and rerun the checks.
The herdr-throwaway-repro skill did not have its intended effect. A disposable session never started, and once it failed there was nothing left to stop the reproduction from running in the user's live session. The skill's only documented way to create a disposable session was `herdr --session <name>` typed into an outer pane. That launches the TUI, and a TUI launched from a Herdr-managed pane exits 1 with "nested herdr is disabled by default" unless the user opted into `experimental.allow_nested`. The rejection was printed inside the scratch pane, which was closed later, while the driving shell only saw 25 seconds of "No such file or directory" from polling for a session socket that was never going to appear. Then it fell back to `herder pane send-text/send-keys/read` with no explicit session and no env prefix, which hit the default session. Fix by starting the disposable session with `herdr --session <name> server`, which is dispatched before the nested gate and needs no config change or outer pane. Address it with the `--session` flag, which marks the session explicit so the inherited HERDR_SOCKET_PATH is ignored and a missing session fails with server_not_running. The HERDR_SESSION variable the skill used for targeting does not do this; it loses to that inherited socket path and answers from the user's session instead.
7708d15 to
ea2dfd3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Got it, done. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.agents/skills/herdr-throwaway-repro/SKILL.md (1)
81-82: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCreate the nested-client configuration with an exclusive temporary path.
printf ... > /var/tmp/<session-name>-config.tomluses a predictable path. It can follow an existing symlink or overwrite an unrelated file. Cleanup can then remove that file.Use
mktemp, retain the exact returned path, pass it throughHERDR_CONFIG_PATH, and remove only that path.Proposed temporary-file handling
-printf '[experimental]\nallow_nested = true\n' > /var/tmp/<session-name>-config.toml +config_path="$(mktemp /var/tmp/herdr-repro-config.XXXXXX)" +printf '[experimental]\nallow_nested = true\n' >"$config_path" -HERDR_CONFIG_PATH=/var/tmp/<session-name>-config.toml +HERDR_CONFIG_PATH="$config_path" -Remove /var/tmp/<session-name>-config.toml +Remove "$config_path"Also applies to: 166-167
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a169859-dd8a-4453-861e-df5357e50602
📒 Files selected for processing (1)
.agents/skills/herdr-throwaway-repro/SKILL.md
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| Choose a short unique name such as `repro-<topic>-<timestamp>`, then prove it is unused before launching: | ||
|
|
||
| Use `/var/tmp` or a dedicated reproduction directory as the new pane's cwd. Save the returned outer pane ID. This is the only parent-session pane that cleanup may close. | ||
| ```bash | ||
| herdr session list --json | ||
| ``` | ||
|
|
||
| ## Start the disposable session | ||
| That lists stopped sessions as well as running ones. A running name is refused, but starting a server on the name of a stopped session silently restores that session's saved workspaces and panes, and cleanup would then delete someone else's session. Pick another name on any exact match. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reserve the session name atomically before launch.
session list and server are separate operations. Another process can create a stopped session after the list check. The server then restores that session, and cleanup deletes it as if it belonged to this reproduction.
Use an atomic name reservation or creation operation. Otherwise, require an ownership token before cleanup; a second list check is not sufficient.
| Choose a short unique name such as `repro-<topic>-<timestamp>`, then prove it is unused before launching: | ||
|
|
||
| Use `/var/tmp` or a dedicated reproduction directory as the new pane's cwd. Save the returned outer pane ID. This is the only parent-session pane that cleanup may close. | ||
| ```bash | ||
| herdr session list --json | ||
| ``` | ||
|
|
||
| ## Start the disposable session | ||
| That lists stopped sessions as well as running ones. A running name is refused, but starting a server on the name of a stopped session silently restores that session's saved workspaces and panes, and cleanup would then delete someone else's session. Pick another name on any exact match. | ||
|
|
||
| Choose a short unique name such as `repro-<topic>-<timestamp>`. | ||
| Start it as a headless server. `herdr --session <name>` launches the TUI, and launching the TUI from inside a Herdr pane exits with `nested herdr is disabled by default` unless the user enabled `experimental.allow_nested`. The `server` command has no such gate. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate and quote session-name before substitution.
The suggested repro-<topic>-<timestamp> format does not constrain <topic>. Whitespace, shell metacharacters, /, or .. can split arguments or escape /var/tmp through the generated configuration path.
Require a safe pattern such as ^[A-Za-z0-9][A-Za-z0-9_-]*$, and quote every substituted value.
Also applies to: 81-82
3 participants
The herdr-throwaway-repro skill did not have its intended effect. A disposable session never started, and once it failed there was nothing left to stop the reproduction from running in the user's live session.
The skill's only documented way to create a disposable session was
herdr --session <name>typed into an outer pane. That launches the TUI, and a TUI launched from a Herdr-managed pane exits 1 with "nested herdr is disabled by default" unless the user opted intoexperimental.allow_nested.The rejection was printed inside the scratch pane, which was closed later, while the driving shell only saw 25 seconds of "No such file or directory" from polling for a session socket that was never going to appear. Then it fell back to
herdr pane send-text/send-keys/readwith no explicit session and no env prefix, which hit the default session.Fix by starting the disposable session with
herdr --session <name> server, which is dispatched before the nested gate and needs no config change or outer pane. Address it with the--sessionflag, which marks the session explicit so the inherited HERDR_SOCKET_PATH is ignored and a missing session fails with server_not_running. The HERDR_SESSION variable the skill used for targeting does not do this; it loses to that inherited socket path and answers from the user's session instead.