fix(pre-code): signal skip via the pre-script output protocol

[waynesun09]

Summary

Phase 3 of the fullsend-ai/fullsend#4718 plan (acked direction): pre-code.sh signals the existing-PR skip through the pre-script output protocol (normative contract, shipped in fullsend-ai/fullsend#5737) instead of GITHUB_OUTPUT.

Supersedes #175 — the skip-flag approach (CODE_SKIP_EXISTING_PR_CHECK / FIX_SKIP_TOOL_INSTALL) was withdrawn after review; no flags, no harness env changes, and local fullsend run behavior is unchanged by construction (@rh-hemartin's concern on #175).

Changes

• pre-code.sh — on finding an open human PR, writes skipped=true + a reason naming the PR to $FULLSEND_PRESCRIPT_OUTPUT, so fullsend run stops before sandbox creation instead of posting the "skipping" comment and then running the agent anyway (the current in-run behavior). The write is guarded on the variable being set: under an older CLI the script behaves exactly as today (fails open, per the protocol's version-skew contract). Proceed paths (no PRs, bot-only PRs, --force, no GH_TOKEN) write nothing — absent skipped means proceed. The GITHUB_OUTPUT writes are removed; they served the inline workflow invocation that fix(#4718)!: remove inline pre-script calls and scaffold script copies fullsend#5739 deletes.
• pre-fix.sh — intentionally untouched. The fix agent has no skip semantics (validation failures and the iteration cap remain hard errors), and its double tool-install disappears with the inline invocation in fix(#4718)!: remove inline pre-script calls and scaffold script copies fullsend#5739.
• scripts/pre-code-test.sh (new) + make script-test wiring — ported from the fullsend scaffold copy that #5739 deletes (so the mock-gh coverage of the existing-PR check survives the single-sourcing), with the GITHUB_OUTPUT assertions replaced by exact-content assertions on the protocol file, plus an explicit old-CLI fails-open case.

Sequencing

Safe to merge now: under the released CLI the guard makes this a no-op (current behavior preserved). It becomes load-bearing once a fullsend release contains #5737 — and it is a merge gate for fullsend-ai/fullsend#5739, which removes the workflows' inline gating.

Testing

• bash scripts/pre-code-test.sh — all 25 cases pass (existing-PR check, bot filtering, force override, protocol writes, old-CLI guard)
• make test — only pre-existing failure is post-scribe-test.sh live-mode-uses-paginate-for-idempotency, which reproduces identically on pristine main (untouched by this PR)
• shellcheck clean on both changed scripts

[qodo-code-review]

PR Summary by Qodo

Signal pre-code skip via pre-script output protocol

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Emit skipped=true + a human-PR reason via the pre-script output protocol for early stop.
• Remove GITHUB_OUTPUT skip signaling; proceed paths write no protocol keys.
• Add mock-gh script tests for existing-PR filtering, force override, and version-skew guard.

Diagram

graph TD
  MF["Makefile (script-test)"] --> PCT["pre-code-test.sh"] --> PCS["pre-code.sh"]
  PCS --> GH{{"gh CLI"}} --> API{{"GitHub API"}}
  PCS --> OUT[("prescript output file")] --> FS["fullsend run"]
  subgraph Legend
    direction LR
    _s["Script"] ~~~ _f[("Protocol file")] ~~~ _e{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep using GITHUB_OUTPUT for skip signaling

• ➕ Minimal change to existing shell workflow patterns
• ➕ Easy to inspect in GitHub Actions step outputs
• ➖ Doesn't integrate with the normative prescript-output contract used by fullsend run
• ➖ Less effective for stopping before sandbox creation (behavior depends on workflow wiring)

2. Move existing-PR skip logic into the `fullsend` CLI

• ➕ Single source of truth for skip semantics and protocol emission
• ➕ Reduces shell-script complexity and mocking surface area
• ➖ Larger change surface and release coordination
• ➖ Harder to iterate quickly compared to a script-only change

3. Always emit an explicit proceed signal (e.g., `skipped=false`)

• ➕ More explicit downstream contract; avoids relying on “absent means proceed”
• ➕ Can simplify consumer logic in some implementations
• ➖ Conflicts with the stated protocol convention in this PR (and increases output noise)
• ➖ Requires ensuring all paths write outputs, increasing risk under set -u and version skew

Recommendation: Proceed with the PR’s protocol-based approach: it aligns with the documented prescript-output contract, enables early termination before sandbox creation, and preserves backwards compatibility via the guarded write (fail-open under older CLIs). The CLI-native alternative is appealing long-term but is a larger coordinated change than needed for this phase.

Files changed (3) +514 / -4

Bug fix (1) +18 / -4

pre-code.shWrite skip decision to prescript output protocol (not GITHUB_OUTPUT) +18/-4

Write skip decision to prescript output protocol (not GITHUB_OUTPUT)

• Adds a 'prescript_output' helper guarded by 'FULLSEND_PRESCRIPT_OUTPUT' to avoid crashing under version skew. On detecting an open human PR, writes 'skipped=true' and a single-line 'reason' naming the PR; removes all 'GITHUB_OUTPUT' skip writes and leaves proceed paths silent (absence of 'skipped' => proceed).

scripts/pre-code.sh

Tests (1) +495 / -0

pre-code-test.shAdd mock-gh regression tests for pre-code skip protocol +495/-0

Add mock-gh regression tests for pre-code skip protocol

• Introduces a bash test harness that mocks 'gh' and exercises the existing-PR detection logic end-to-end via the real '--jq' filter. Adds assertions for protocol output ('FULLSEND_PRESCRIPT_OUTPUT') including bot PR filtering, force override bypass, no-token behavior, and old-CLI fail-open when the env var is unset.

scripts/pre-code-test.sh

Other (1) +1 / -0

MakefileRun pre-code script tests in 'make script-test' +1/-0

Run pre-code script tests in 'make script-test'

• Adds 'scripts/pre-code-test.sh' to the 'script-test' target so the new harness runs in CI/local script test suites.

Makefile

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:40 PM UTC · Completed 9:56 PM UTC
Commit: f498723 · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (1)

Context used

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

1. Protected scripts/ files modified 📜 Skill insight § Compliance

Description

This PR modifies files under the protected governance/infrastructure path scripts/, which must not
be auto-approved and requires explicit human review/approval even when justified. Ensure the
required CODEOWNERS reviewers approve before merge.

Code

scripts/pre-code.sh[R21-27]

+# prescript_output KEY VALUE — append a protocol line, if the CLI
+# supports the protocol. Values must be single-line (protocol grammar).
+prescript_output() {
+  if [[ -n "${FULLSEND_PRESCRIPT_OUTPUT:-}" ]]; then
+    printf '%s=%s\n' "$1" "$2" >> "${FULLSEND_PRESCRIPT_OUTPUT}"
+  fi
+}

Relevance

●●● Strong

Protected-path enforcement is explicit; schema rejects approve when protected-path findings exist.

PR-#303

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance checklist flags any modifications under protected paths (including scripts/) as
requiring a finding and human review to prevent auto-approval. This PR updates scripts/pre-code.sh
and adds scripts/pre-code-test.sh, both under scripts/.

scripts/pre-code.sh[21-27]
scripts/pre-code-test.sh[1-8]
Skill: pr-review

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
This PR changes protected governance/infrastructure paths (under `scripts/`). Per compliance, such PRs must not be auto-approved and must receive explicit human approval.

## Issue Context
The change is justified by the linked issue (#4718) in the PR description, but protected-path modifications still require human review before merge.

## Fix Focus Areas
- scripts/pre-code.sh[21-27]
- scripts/pre-code-test.sh[1-8]
- Makefile[47-52]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Stale protocol skip state 🐞 Bug ≡ Correctness

Description

scripts/pre-code.sh only appends to FULLSEND_PRESCRIPT_OUTPUT on the skip path and never
clears/truncates the protocol file on proceed paths, so a reused/non-empty output file can retain a
previous skipped=true and incorrectly skip a later run. Reset/truncate the protocol file once at
script start when FULLSEND_PRESCRIPT_OUTPUT is set so “proceed” deterministically leaves it empty.

Code

scripts/pre-code.sh[R21-26]

+# prescript_output KEY VALUE — append a protocol line, if the CLI
+# supports the protocol. Values must be single-line (protocol grammar).
+prescript_output() {
+  if [[ -n "${FULLSEND_PRESCRIPT_OUTPUT:-}" ]]; then
+    printf '%s=%s\n' "$1" "$2" >> "${FULLSEND_PRESCRIPT_OUTPUT}"
+  fi

Relevance

●● Moderate

No clear prior on truncating protocol files; team recently rejected stale pre-existing file concern
elsewhere.

PR-#508

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The script enables set -euo pipefail and defines prescript_output() to always append (>>) when
FULLSEND_PRESCRIPT_OUTPUT is set; it is only invoked on the skip path, while the proceed path writes
nothing. The new unit test contract explicitly states proceed paths should leave the protocol file
empty, which is not guaranteed if the file already has prior contents.

scripts/pre-code.sh[19-27]
scripts/pre-code.sh[131-138]
scripts/pre-code-test.sh[388-392]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pre-code.sh` writes protocol keys using append redirection (`>>`) and never resets the protocol output file on non-skip (proceed) paths. If the caller reuses an existing output file path across runs without truncating it, stale `skipped=true` can persist and be misinterpreted for a new run.

### Issue Context
The repo’s own test contract/comment states that proceed paths must leave the protocol file empty (absent `skipped` means proceed). Ensuring the script truncates the file when `FULLSEND_PRESCRIPT_OUTPUT` is set makes this behavior deterministic and prevents state leakage.

### Fix Focus Areas
- scripts/pre-code.sh[19-27]
- scripts/pre-code.sh[131-138]
- scripts/pre-code-test.sh[388-392]

### Implementation notes
- Near the top of `pre-code.sh` (after `set -euo pipefail`), add something like:
 - `if [[ -n "${FULLSEND_PRESCRIPT_OUTPUT:-}" ]]; then : >"${FULLSEND_PRESCRIPT_OUTPUT}"; fi`
- Keep `prescript_output()` as an append helper, but rely on the initial truncation so the file represents only the current invocation.
- (Optional) Add/extend a unit test case to cover the “file pre-populated with skipped=true, then proceed path should empty it” scenario.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] scripts/pre-code.sh, scripts/pre-code.src.sh, scripts/lib/prescript-output.lib.sh, scripts/pre-code-test.sh — Protected paths modified (under scripts/). The PR references Remove redundant pre-code.sh / pre-fix.sh calls from reusable workflows fullsend#4718 with acked direction and provides thorough rationale for the protocol migration and test addition. Human approval is always required for protected-path changes, regardless of context.

Low

• [incomplete-documentation] docs/code.md:25 — The --force flag is mentioned without explaining its purpose (overriding the existing-PR check), and the pre-script's skip behavior is described only as "checks for open PRs" without detailing consequences. This is a pre-existing documentation gap — the PR changed internal signaling, not user-facing behavior.

• [missing-documentation] README.md:37 — The pre-script section describes pre-scripts as "validate inputs and prepare the environment" without documenting the FULLSEND_PRESCRIPT_OUTPUT signaling protocol. The protocol is documented inline in the library file header and script comments. The README is a high-level overview, not a protocol reference.

Previous run

Review

Findings

Medium

• [protected-path] scripts/pre-code.sh, scripts/pre-code-test.sh — Protected paths modified: scripts/pre-code.sh and scripts/pre-code-test.sh (under scripts/). The PR provides sufficient context — references Remove redundant pre-code.sh / pre-fix.sh calls from reusable workflows fullsend#4718 with acked direction, explains rationale and sequencing thoroughly. Human approval is always required for protected-path changes, regardless of context.

• [undocumented-architectural-pattern] scripts/pre-code.sh:19 — Introduces the prescript_output() helper and FULLSEND_PRESCRIPT_OUTPUT protocol for the first time in this codebase. The header comment (lines 7–14) documents the protocol inline with a reference to the normative spec, but there is no AGENTS.md or ADR entry providing a centralized reference for the pattern. Consider adding a brief entry so future contributors can discover the protocol without reading the script source.

• [missing-documentation] README.md:37 — The pre-script section describes pre-scripts as validating inputs and preparing the environment but does not document the pre-script output protocol. The old GITHUB_OUTPUT mechanism was also undocumented in the README, so this is not a regression — but it is an opportunity to document the signaling contract as the protocol matures.

Low

• [test-adequacy] scripts/pre-code-test.sh:107 — The run_test, run_test_stdout, and run_test_stdout_excludes helpers still set GITHUB_OUTPUT in the test environment, but the production code no longer writes to GITHUB_OUTPUT. The dead setup is harmless but may mislead future maintainers into thinking GITHUB_OUTPUT is still part of the contract.

• [stale-reference] docs/code.md:50 — The pre-script description mentions checking for open PRs but does not explain the skip signal mechanism. The description is accurate at its current abstraction level but could be expanded to mention the protocol-based skip signal.

---

Labels: PR modifies scripts/pre-code.sh, the pre-script for the code agent pipeline

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:07 PM UTC · Ended 6:17 PM UTC
Commit: dcd4f1d · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:19 PM UTC · Ended 6:20 PM UTC
Commit: 670df8f · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:22 PM UTC · Completed 6:33 PM UTC
Commit: f35dc97 · View workflow run →

[ralphbean]

LGTM

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:37 PM UTC · Completed 6:52 PM UTC
Commit: f35dc97 · View workflow run →

[fullsend-ai-retro]

Retro: PR #536 — fix(pre-code): signal skip via the pre-script output protocol

Workflow overview: Human-authored PR by waynesun09, reviewed by two bot reviewers (qodo-code-review, fullsend-ai-review) and two humans (ifireball, ralphbean). Merged after one round of human feedback across 4 commits over ~21 hours.

Timeline

1. PR opened (Jul 29, 21:38). First review agent run (30493170389) completed in 17m40s, dispatching all 6 sub-agents. Challenger correctly removed a critical false positive (missing-authorization from intent-coherence) and a speculative finding, downgraded 5 others. Final output: 2 medium, 5 low findings.
2. ifireball requested changes (Jul 30, 18:08): suggested extracting prescript_output() to a shared library, following the repo's existing bundling pattern.
3. Author addressed feedback in commit 670df8f, extracting the helper to scripts/lib/prescript-output.lib.sh and adopting the make script-build bundling convention.
4. Second review run (30569983334) completed in 12m40s ($3.92). Re-review correctly skipped 2 sub-agents (security, cross-repo-contracts) and resolved both prior medium findings.
5. ifireball approved (18:23), ralphbean approved (18:34), merged (18:35).

What went well

• Challenger adjudication worked precisely — removed a critical false-positive missing-authorization and a speculative protocol-adoption-sequencing, downgraded 5 others. Without the challenger, the critical false positive would have triggered a request-changes review on a correctly-authorized PR.
• Re-review optimization was effective: 4 sub-agents instead of 6, correct prior-finding resolution, ~30% faster and cheaper than the first run.
• Post-script behavior correctly updated the existing sticky comment in-place and minimized stale reviews.
• Author responses to agent findings were substantive: converted the dead GITHUB_OUTPUT test setup into active regression assertions (addressing the test-adequacy finding) and added centralized protocol documentation (addressing the undocumented-pattern finding).

Review gap

The review agent's intent-coherence sub-agent flagged undocumented-architectural-pattern (documentation gap). The human reviewer identified a different concern on the same code: the helper should be extracted to a shared library following the repo's existing scripts/lib/ + bundling convention. The style-conventions sub-agent, which explicitly owns "code organization" and is instructed to "derive patterns from the existing codebase," did not flag this structural non-conformance. See proposal below.

Observations covered by existing issues

• 7 cancelled review runs during the rapid push window (18:05–18:35) — covered by fullsend#4069, fullsend#4960, fullsend#5139, and agents#323.
• requires-manual-review label applied despite human review already being in progress — covered by agents#304 and agents#395.
• Issue agents#333 covers inline reimplementation detection (related but distinct: different sub-agent, different concern type — duplication vs. organizational conformance).

Proposals filed

• Style-conventions sub-agent should check code factoring against repo shared-library patterns (in fullsend-ai/agents)