Merge pull request #1933 from Hack23/copilot/fix-agentic-workflows

Document sandbox-blocked shell expansion patterns in prompt shell-safety guide

Author: pethers

.github/prompts/00-base-contract.md

@@ -45,6 +45,63 @@ Analysis Gate → Article (if applicable) → Stage → Commit → ONE create_pu
 
 No step may be skipped, reordered, or executed in parallel with its successor.
 
+## Phase checkpoint — persist every phase to repo memory
+
+Valuable analysis must never be lost. After each pipeline phase completes, snapshot its output to the gh-aw repo-memory mount at `$GH_AW_MEMORY_DIR` (runtime default `/tmp/gh-aw/repo-memory/default`). gh-aw pushes that directory to the `memory/news-generation` branch in a **separate post-job** — so checkpoints survive even if the content PR job fails, crashes, or times out.
+
+### Mandatory checkpoint points
+
+| After phase | Phase label | Source(s) |
+|-------------|-------------|-----------|
+| 03 Data download | `phase-03-download` | `$ANALYSIS_DIR` (manifest + fetched data summaries) |
+| 04 Analysis Pass 1 | `phase-04-pass1` | `$ANALYSIS_DIR` top-level artifacts |
+| 04 Analysis Pass 2 | `phase-04-pass2` | `$ANALYSIS_DIR` top-level artifacts |
+| 05 Gate pass | `phase-05-gate` | `$ANALYSIS_DIR` top-level artifacts |
+| 06 Article generated | `phase-06-article` | `$ANALYSIS_DIR` + today's `news/${ARTICLE_DATE}-*.html` |
+| 07 Immediately before `create_pull_request` | `phase-07-final` | `$ANALYSIS_DIR` + articles from `news/${ARTICLE_DATE}-*.html` |
+| `news-translate` per batch | `phase-translate-<lang>` | Translated `news/${ARTICLE_DATE}-*.html` |
+
+Each checkpoint is mandatory. Skipping them forfeits the only cross-run safety net for analysis work.
+
+### Reusable snippet
+
+Run this bash block at the end of every phase (pass the phase label as `$1`). Article HTML is written directly under the flat `news/` directory, so checkpoint copies must use `news/${ARTICLE_DATE}-*.html` rather than `news/$YYYY/$MM/$DD/*.html`:
+
+```bash
+set -Eeuo pipefail
+: "${GH_AW_MEMORY_DIR:=/tmp/gh-aw/repo-memory/default}"
+: "${ARTICLE_DATE:?ARTICLE_DATE required for checkpoint}"
+: "${SUBFOLDER:?SUBFOLDER required for checkpoint (use batch/<lang> for news-translate)}"
+PHASE="${1:?phase label required, e.g. phase-04-pass1}"
+ANALYSIS_DIR="${ANALYSIS_DIR:-analysis/daily/$ARTICLE_DATE/$SUBFOLDER}"
+DEST="$GH_AW_MEMORY_DIR/$ARTICLE_DATE/$SUBFOLDER/$PHASE"
+mkdir -p "$DEST" 2>/dev/null || { echo "[checkpoint] mkdir failed for $DEST — continuing"; exit 0; }
+# Snapshot top-level analysis artifacts (never documents/ — often 100+ files — and never pass1/).
+if [ -d "$ANALYSIS_DIR" ]; then
+  find "$ANALYSIS_DIR" -maxdepth 1 -type f \( -name '*.md' -o -name '*.json' \) \
+    -exec cp -f {} "$DEST"/ \; 2>/dev/null || true
+fi
+# Snapshot today's produced article HTML from the flat news/ directory (if any exists at this phase).
+if [ -d "news" ]; then
+  find "news" -maxdepth 1 -type f -name "${ARTICLE_DATE}-*.html" \
+    -exec cp -f {} "$DEST"/ \; 2>/dev/null || true
+fi
+COUNT="$(find "$DEST" -maxdepth 1 -type f 2>/dev/null | wc -l | tr -d ' ')"
+echo "[checkpoint] $PHASE → $DEST ($COUNT files)"
+exit 0
+```
+
+### Checkpoint rules
+
+| Rule | Rationale |
+|------|-----------|
+| **Never block on checkpoint failure** — always `exit 0`. | Repo-memory is a safety net, not a gate. |
+| Do **not** copy `$ANALYSIS_DIR/documents/` or `$ANALYSIS_DIR/pass1/`. | `documents/` exceeds the 50-file push cap; `pass1/` is local gate evidence only. |
+| Do **not** stage or commit anything under `$GH_AW_MEMORY_DIR`. | gh-aw's `push_repo_memory` post-job publishes it; see `07-commit-and-pr.md`. |
+| Prefer small summary `.md` / `.json` files (≤ 50 KB each, ≤ 50 per push). | gh-aw silently drops files exceeding the push caps. |
+| Re-run the snippet at every phase, even if earlier phases already snapshotted — it overwrites with the latest content. | Ensures the final state is always preserved, and earlier snapshots remain on the branch from prior runs. |
+| For `news-translate`, use `SUBFOLDER=batch/<lang-or-batch-id>` so memory paths don't collide with analysis runs. | Keeps the branch organised by article type. |
+
 ## Output contract
 
 - Commit real files on disk under `analysis/daily/` and/or `news/`.

.github/prompts/01-bash-and-shell-safety.md

@@ -32,10 +32,25 @@ bash({
 
 Parameter expansion (`${VAR}`, `${VAR:-x}`, `${VAR##*/}`, …) and command substitution (`$(cmd)`) are **safe** under the agentic-workflow firewall — the firewall inspects outbound network egress, not shell syntax. Process substitution `<(…)` is best avoided because some runners disable `/dev/fd`.
 
+## Banned expansion patterns (sandbox blocklist)
+
+The execution sandbox **rejects** commands containing any of the following patterns before they run. Rewrite using the safe equivalent instead of trying to work around the block.
+
+| Banned pattern | Why it's blocked | Safe equivalent |
+|----------------|------------------|-----------------|
+| `${var@P}` / `${var@Q}` / `${var@E}` / `${var@A}` / `${var@a}` | These parameter transformations can produce shell-reparsable fragments or syntax-bearing representations, which can smuggle attacker-controlled content into later parsing steps — a known prompt-injection vector. | Expand the target explicitly: `printf '%s' "$var"`, or a plain `"$var"` substitution. |
+| `${!var}` | Indirect expansion uses `$var`'s *value* as another variable name — lets an attacker-controlled string pick which variable is read. | Use an associative array: `declare -A MAP; MAP[foo]=bar; echo "${MAP[$key]}"`. |
+| Nested `$(…$(…)…)` | Builds a command string dynamically from inner results — the classic staged injection shape. | Split into two lines with a temporary variable: `inner=$(cmd2); outer=$(cmd1 "$inner")`. |
+| Chained builder assignments that progressively construct a command substitution (`a=foo; b="$a"bar; c=$($b)`) | Same staged-injection shape, just spread across multiple statements. | Construct commands as arrays, invoke via `"${cmd[@]}"`; never re-parse a string as a command. |
+| `eval` on variable contents (or eval-like constructs such as `bash -c "$var"`, `source /dev/stdin <<<"$var"`) | Direct arbitrary-code execution from data. | Never required for our workflows — refuse and rewrite using arrays, `case`, or explicit branches. |
+
+These rules apply equally to inline bash in prompts AND to bash commands the agent composes at runtime. The sandbox rejects matching commands before they run.
+
 ## Secret safety
 
 - Never pass secrets through `$(…)` into a log-visible command — echoing `curl -H "Authorization: $(…)"` will leak if the step is rerun in debug.
-- Use env blocks (`env:` on the step) or `${{ secrets.FOO }}` directly; the runner masks secret values in output.
+- Expose secrets through the step's `env:` block (for example `env: { FOO: <GitHub-Actions secrets expression for FOO> }`) rather than inlining a raw secrets expression inside the prompt; the runner masks secret values in output.
+- Note: this prompt file is loaded via `runtime-import`, and the gh-aw validator rejects any GitHub Actions template expression (the double-curly-brace syntax used for `secrets`, `env`, `inputs`, etc. in workflow YAML) that is not on the safe allow-list — so never embed such an expression in prompt modules, even inside code spans. Keep secret references in the workflow YAML only.
 
 ## Temporary files
 

.github/prompts/03-data-download.md

@@ -69,3 +69,5 @@ Always produce `analysis/daily/$ARTICLE_DATE/$SUBFOLDER/data-download-manifest.m
 ## Next step
 
 On success, proceed to `04-analysis-pipeline.md`. Never start analysis while `data-download-manifest.md` is missing or empty.
+
+After the manifest is written, run the **phase checkpoint** from `00-base-contract.md` with label `phase-03-download` so the download provenance is persisted to repo memory before any analysis begins.

.github/prompts/04-analysis-pipeline.md

@@ -66,7 +66,9 @@ For each article with charts, produce accompanying JSON under `analysis/daily/$A
 
 ## Next step
 
-On completion, proceed to `05-analysis-gate.md`. Do not start article generation until the gate passes.
+Proceed to `05-analysis-gate.md`. Do not start article generation until the gate passes.
+
+After completing Pass 1 (before Pass 2), run the **phase checkpoint** from `00-base-contract.md` with label `phase-04-pass1`. After completing Pass 2 (before the gate), run it again with label `phase-04-pass2`. This guarantees both iterations survive even if the gate, article, or commit phase later fails.
 
 ## External references
 

.github/prompts/05-analysis-gate.md

@@ -153,7 +153,7 @@ Exit code 0 = pass, non-zero = fail with per-check report. Precondition for chec
 
 ## Outcome
 
-- **Pass** → proceed to `06-article-generation.md`.
+- **Pass** → run the **phase checkpoint** from `00-base-contract.md` with label `phase-05-gate`, then proceed to `06-article-generation.md`.
 - **Fail** → fix flagged files (never delete them), re-run the gate, then proceed.
 - **Unrecoverable fail after fixes** → stage whatever analysis exists, commit with label `analysis-only`, call `safeoutputs___create_pull_request` once (see `07-commit-and-pr.md`). Do **not** generate articles.
 

.github/prompts/06-article-generation.md

@@ -84,4 +84,4 @@ Each article ≥ 1000 words, minimum 3 of 5 mandatory analytical sections presen
 
 ## Next step
 
-Stage all analysis + article + visualisation files, then call `07-commit-and-pr.md`.
+Run the **phase checkpoint** from `00-base-contract.md` with label `phase-06-article` to persist the generated articles to repo memory. Then stage all analysis + article + visualisation files, and call `07-commit-and-pr.md`.

.github/prompts/07-commit-and-pr.md

@@ -89,6 +89,12 @@ Call `safeoutputs___noop({"message": "<reason>"})` **only** if:
 
 In every other case, commit whatever exists and call `create_pull_request` once.
 
+## Final checkpoint — before the PR call
+
+Immediately before calling `safeoutputs___create_pull_request`, run the **phase checkpoint** from `00-base-contract.md` with label `phase-07-final`. This snapshots the final authoritative analysis + article state to repo memory, so even if the PR call, the safe-outputs runner, or the post-job push fails, the last good state survives on the `memory/news-generation` branch.
+
+For `news-translate`, run the checkpoint with label `phase-translate-<lang>` after each per-language batch succeeds (before the final PR call), so individual language translations are preserved even if later languages fail.
+
 ## Deadline enforcement
 
 If the run exceeds 40 minutes with no safe-output call yet: