fix: process trace Phase 2 — trap chaining, stack flush, portability#173
Open
fentas wants to merge 5 commits into
Open
fix: process trace Phase 2 — trap chaining, stack flush, portability#173fentas wants to merge 5 commits into
fentas wants to merge 5 commits into
Conversation
- Chain DEBUG/EXIT traps with existing handlers instead of overwriting - Flush pending stack entries in exit hook (catches missed exits) - Use printf instead of echo for trace output (portability) - Remove unsafe return/exit guard — use if/else/fi structure - Fix version in docs (v0.11.0 not v0.9.0) Addresses Copilot review on #168 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR iterates on the ARGSH_TRACE “process trace” feature (Phase 2) to make tracing safer to enable in real scripts by avoiding top-level guard returns (for minified bundling), improving portability of trace writes, and attempting to preserve existing DEBUG/EXIT traps.
Changes:
- Replace the top-level
return/exitguard with anif/else/fiblock so the file can be safely bundled/minified. - Switch trace line writes from
echotoprintf, and flush any remaining stack frames in the EXIT hook. - Update documentation to reflect the correct version where
ARGSH_TRACEwas introduced.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| libraries/trace.sh | Trap chaining changes, printf-based writes, and EXIT-time stack flush for ARGSH_TRACE. |
| docs/environment-variables.mdx | Updates the “New in …” version for ARGSH_TRACE. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add `argsh-dap --trace <out.md> -- <script> [args...]` that runs a bash script with the DEBUG trap prelude, collects all function entry/exit events via FIFO, then writes a structured markdown trace enriched with argsh_syntax analysis (field types, command tree, import tree). - TRACE_PRELUDE: fire-and-forget DEBUG trap (no blocking on ctl FIFO) - run_trace_mode(): FIFO setup, event collection, markdown rendering - render_trace_markdown(): builds header, command tree, execution table, args details, import tree, and summary sections - 5 integration tests covering output format, function tracing, error cases, and non-zero exit code reporting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…feature Add 6 new test cases for ARGSH_TRACE covering header fields, function entry/exit recording, :args variable dumps, summary section validation, non-zero exit codes, and EXIT trap chaining. Fix existing 3 trace tests to use standalone scripts instead of subshells (avoids bats DEBUG trap conflicts). Create docs/development/tools/trace.mdx documenting both ARGSH_TRACE env var (Phase 1) and argsh-dap --trace (Phase 2) methods with example output and use cases. Update debugger.mdx to cross-reference trace mode and add trace page to sidebars.js. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Hold TempDir for automatic cleanup on all exit paths (no .keep()) - Remove unused _exit_code_fifo (process exit code used instead) - Scan first 10 lines for argsh detection (not just shebang) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ection - Preserve $? for chained EXIT handlers via save/restore pattern - Route stack flush through __argsh_trace_write for consistency - Cap markdown indent at 1 level to avoid CommonMark code blocks - Tighten argsh detection: check shebang + import/source patterns Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment on lines
+1840
to
+1841
| let cmd_display = if step.command.len() > 60 { | ||
| format!("{}...", &step.command[..57]) |
Comment on lines
+386
to
+389
| ( | ||
| flock "${__ARGSH_TRACE_LOCK}" | ||
| printf 'TRACE_EXIT\t%d\n' "$?" > "${__ARGSH_TRACE_FIFO}" | ||
| ) |
| source "\$1/args.sh" | ||
|
|
||
| # Set an EXIT trap BEFORE sourcing trace.sh | ||
| trap "touch ${_marker}" EXIT |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2 of process trace (#99):
Fixes from Phase 1 Copilot review:
Phase 2: argsh-dap --trace
argsh-dap --trace output.md -- script.sh [args]— headless execution tracingTests:
Docs:
docs/development/tools/trace.mdx— full trace feature documentationCloses #99
🤖 Generated with Claude Code