Skip to content

feat(#4718): add pre-script output protocol to fullsend run - #5737

Merged
waynesun09 merged 2 commits into
mainfrom
4718-prescript-skip-protocol
Jul 29, 2026
Merged

feat(#4718): add pre-script output protocol to fullsend run#5737
waynesun09 merged 2 commits into
mainfrom
4718-prescript-skip-protocol

Conversation

@waynesun09

Copy link
Copy Markdown
Member

Summary

CLI half of the direction agreed on #4718 (comment, acked by @ifireball, @rh-hemartin, @ggallen): pre-scripts get a first-class skip protocol inside fullsend run, so the reusable workflows' inline pre-script calls — the source of the double execution, and the last consumer of the scaffold script copies (#5667) — become removable in a follow-up PR.

Related Issue

Part of #4718 (also unblocks #5667, closed as duplicate into this effort).

Changes

  • New internal/prescript package owning the protocol: fullsend run exports FULLSEND_PRESCRIPT_OUTPUT (temp file path) into the pre-script env; the script may append GitHub-output-style key=value lines (skipped=true, optional reason=...). Missing/empty file → proceed (today's behavior). Malformed content is a hard error so a mistyped skip cannot silently proceed as a duplicate agent run.
  • internal/cli/run.go: pre-script block extracted into a runPreScript helper wired to the protocol; on skipped=true the run reports a skipped status, relays outputs to GITHUB_OUTPUT (when under GHA; auto-detected in the CLI, not hard-coded in workflow YAML — portable per the review discussion on docs(#4718): add ADR 0072 for the pre-script output protocol #5016), and exits 0 before sandbox creation.
  • internal/statuscomment: new skipped completion status (⏭️).

Version skew (ADR 0062): both directions degrade to current behavior — scripts unaware of the protocol proceed as before; protocol-aware scripts must guard on the env var being unset under older CLIs (same pattern as the existing GITHUB_OUTPUT guard).

Follow-ups (sequenced on #4718): workflow cleanup + scaffold script deletion (resolves #5667), agents-repo script changes (recut of fullsend-ai/agents#175), ADR rewrite (#5016).

Testing

  • make lint passes (staged)
  • Tests added/updated for new or modified logic — full coverage of internal/prescript, runPreScript exercised with fixture scripts (skip, proceed, runner-env, script failure, malformed output), statuscomment skipped-status test
  • make go-vet, go build ./..., go test -race on changed packages; full go test ./... run — one pre-existing failure on pristine origin/main unrelated to this diff (TestListTriggeredHarnesses_BaseComposition, macOS temp-dir symlink vs workspace-root escape check)
  • make e2e-test not run locally (requires live pool orgs + mint OIDC); covered by CI

Checklist

  • PR title follows Conventional Commits (correct type, ! for breaking changes)
  • Commits are signed off (DCO) — human and human-directed agent sessions only
  • I wrote this contribution myself and can explain all changes in it

@waynesun09
waynesun09 requested a review from a team as a code owner July 29, 2026 17:57
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add pre-script skip protocol to fullsend run

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Add FULLSEND_PRESCRIPT_OUTPUT protocol so pre-scripts can request run skip.
• Exit before sandbox creation and relay outputs to GITHUB_OUTPUT on skip.
• Introduce skipped completion status and unit tests for protocol and CLI flow.
Diagram

graph TD
  A["fullsend run (runAgent)"] --> B["runPreScript()"] --> C["Pre-script (host)"] --> D[("Prescript output file")] --> E["prescript.ParseFile"] --> F{"Skipped?"}
  F --> G["Create sandbox"]
  F --> H["CI reporting (Relay + status)"]
  subgraph Legend
    direction LR
    _p["Process"] ~~~ _f[("File")] ~~~ _d{"Decision"}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use a dedicated exit code to signal skip
  • ➕ Simpler than file parsing; no temp file needed
  • ➕ Works even without CI env files
  • ➖ Hard to attach structured metadata like reason/extra outputs
  • ➖ Exit codes can conflict with existing scripts and error handling; less explicit/portable
2. Parse a structured marker from stdout/stderr
  • ➕ No temp file; easy for scripts to emit
  • ➕ Works across many environments
  • ➖ Fragile: output mixing/log noise can break parsing
  • ➖ Harder to make malformed content a reliable hard error without false positives
3. Write directly to GITHUB_OUTPUT from the pre-script
  • ➕ Leverages existing GitHub Actions mechanism; no new protocol surface
  • ➖ Ties behavior to GitHub Actions; non-GHA runs lose capability
  • ➖ Reintroduces workflow-level coupling the PR is trying to eliminate

Recommendation: Keep the temp-file env-var protocol: it’s explicit, portable outside GitHub Actions, supports structured outputs (reason + future keys), and allows strict validation so a mistyped skip cannot silently proceed. The relay step preserves CI integration without hard-coding workflow YAML behavior.

Files changed (6) +523 / -11

Enhancement (3) +243 / -11
run.goWire pre-script skip protocol into 'fullsend run' and add helper +59/-11

Wire pre-script skip protocol into 'fullsend run' and add helper

• Refactors pre-script execution into a 'runPreScript' helper that prepares the output file, executes the script, and parses results. Adds early-exit behavior when 'skipped=true', relays outputs to GitHub Actions when available, and reports a new "skipped" completion status via the existing status notification defer.

internal/cli/run.go

prescript.goAdd 'internal/prescript' package for skip protocol parsing and relay +182/-0

Add 'internal/prescript' package for skip protocol parsing and relay

• Implements the FULLSEND_PRESCRIPT_OUTPUT contract: create a temp output file, parse GitHub-output-style key=value lines with strict validation and size limits, and expose outputs to GITHUB_OUTPUT when present. Normalizes 'skipped' to true/false and treats malformed content as a hard error.

internal/prescript/prescript.go

statuscomment.goAdd skipped status emoji mapping +2/-0

Add skipped status emoji mapping

• Extends status-to-emoji mapping to support a new "skipped" completion state.

internal/statuscomment/statuscomment.go

Tests (3) +280 / -0
prescript_run_test.goAdd runPreScript integration tests with fixture shell scripts +78/-0

Add runPreScript integration tests with fixture shell scripts

• Introduces tests that create executable pre-scripts to validate skip/proceed behavior, runner-env propagation, and hard-error handling for script failures and malformed outputs. Skips the suite on Windows where POSIX shell isn’t available.

internal/cli/prescript_run_test.go

prescript_test.goTest pre-script output parsing, validation, and GITHUB_OUTPUT relay +180/-0

Test pre-script output parsing, validation, and GITHUB_OUTPUT relay

• Adds coverage for missing/empty outputs, CRLF/comments, last-write-wins semantics, invalid syntax/key/value errors, oversize protection, and relay behavior (append + deterministic ordering).

internal/prescript/prescript_test.go

statuscomment_test.goAdd notifier test for skipped completion rendering +22/-0

Add notifier test for skipped completion rendering

• Adds a test asserting that completion comments include the "⏭️ Skipped" status when the notifier is told the run completed as skipped.

internal/statuscomment/statuscomment_test.go

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 5:58 PM UTC · Ended 6:00 PM UTC
Commit: 2aa519f · View workflow run →

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.59060% with 17 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/prescript/prescript.go 84.82% 10 Missing and 7 partials ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure · Started 5:58 PM UTC · Completed 6:00 PM UTC
Commit: 2aa519f · View workflow run →

@qodo-code-review

qodo-code-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 54 rules

Grey Divider


Action required

1. Output keys reject hyphens ✓ Resolved 🐞 Bug ≡ Correctness
Description
internal/prescript.ParseFile validates output keys with ^[a-zA-Z_][a-zA-Z0-9_]*$, which rejects
hyphenated keys even though GitHub Actions output names commonly include '-' (and this repo already
emits such keys). Because runPreScript treats ParseFile errors as hard failures, a pre-script
writing e.g. install-method=... will fail the run instead of relaying outputs.
Code

internal/prescript/prescript.go[52]

+var validKeyRe = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`)
Relevance

●●● Strong

Team often broadens regexes to match real-world allowed chars; hyphen support already accepted in
similar validations.

PR-#390
PR-#736

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The parser enforces an underscore-only key grammar and errors on any other character; runPreScript
treats that error as fatal. The repo’s own GitHub Actions outputs demonstrate hyphenated keys are in
active use, so a pre-script following those conventions would now break.

internal/prescript/prescript.go[52-53]
internal/prescript/prescript.go[113-121]
internal/cli/run.go[2277-2303]
action.yml[95-99]
action.yml[131-139]

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

## Issue description
`internal/prescript.ParseFile` rejects output keys containing `-`, but the protocol is described as GitHub-output-style and GitHub Actions outputs commonly use hyphenated names (e.g. `install-method`, `cache-hit`). This makes protocol-aware pre-scripts unexpectedly fail with “invalid key”.

## Issue Context
- The parser enforces `^[a-zA-Z_][a-zA-Z0-9_]*$` and errors on mismatch.
- `runPreScript` converts any parse error into a hard run failure.
- The repository already writes hyphenated output names to `GITHUB_OUTPUT` in `action.yml`.

## Fix Focus Areas
- internal/prescript/prescript.go[52-52]
- internal/prescript/prescript.go[113-121]
- internal/prescript/prescript_test.go[80-88]

## Suggested fix
1. Loosen `validKeyRe` to allow hyphens after the first character (e.g. `^[a-zA-Z_][a-zA-Z0-9_-]*$`).
2. Add/adjust a unit test proving a hyphenated key parses (and can be relayed), e.g. `install-method=vendored`.
3. (Optional) Update the package comment to explicitly document the allowed key charset.

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


Grey Divider

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

Qodo Logo

Comment thread internal/prescript/prescript.go Outdated
@waynesun09
waynesun09 force-pushed the 4718-prescript-skip-protocol branch from 2aa519f to 9584a08 Compare July 29, 2026 18:33
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 6:34 PM UTC · Ended 6:50 PM UTC
Commit: 9584a08 · View workflow run →

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

Site preview

Preview: https://0b1908a3-site.fullsend-ai.workers.dev

Commit: d08da4951b4e9476a104de45344af5a7253de0c4

@waynesun09
waynesun09 force-pushed the 4718-prescript-skip-protocol branch from 9584a08 to a012fbe Compare July 29, 2026 18:49
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 6:51 PM UTC · Ended 6:58 PM UTC
Commit: a012fbe · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 6:59 PM UTC · Ended 7:06 PM UTC
Commit: eded4af · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:08 PM UTC · Completed 7:22 PM UTC
Commit: 9d49198 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [edge-case] internal/prescript/prescript.go:143bufio.Scanner splits on \n only; a file using bare CR line terminators would be treated as a single long line. The code handles CRLF via TrimRight(scanner.Text(), "\r"), but a CR-only file would be parsed as one big line. Bare CR-only line endings are not generated by any modern tool or shell, so this is cosmetic.

  • [edge-case] internal/prescript/prescript.go:144 — The scanner buffer maximum (1 MiB) matches the file size cap from the stat check, so the error-message difference for a too-long line versus an oversized file is cosmetic at most.

  • [github-action-contract] action.yml — Two new action outputs (skipped and skip-reason) are added to the composite action. This is a backward-compatible, additive change — existing consumers are unaffected. The three-state contract (true/false/empty) is well-documented in the new normative spec.

  • [cli-protocol-contract] docs/normative/prescript-output/v1/README.md — New normative protocol specification establishes a versioned contract between fullsend run and harness pre-scripts. Version skew handling is asymmetric by design (fail-safe vs fail-open), consistent with ADR 0062.

Previous run

Review

Findings

Low

  • [edge-case] internal/prescript/prescript.go:150bufio.Scanner splits on \n only; a file using bare CR line terminators would be treated as a single long line. The normative doc's characterization of \r as a line terminator refers to the GITHUB_OUTPUT injection risk in values, not the parser's own line splitting. Bare CR-only line endings are not generated by any modern tool.

  • [edge-case] internal/prescript/prescript.go:146 — The scanner buffer maximum (1 MiB) matches the file size cap from the stat check, so the error-message difference for a too-long line is cosmetic at most.

  • [test-naming] internal/cli/prescript_run_test.go:351 — Comment references useFakeOpenshell but the actual helper function is named usePreScriptStub. Appears to be a copy-paste artifact from the existing useFakeOpenshell pattern in run_test.go.

  • [incomplete-documentation] docs/ADRs/0024-harness-definitions.md:110 — The run lifecycle diagram does not mention that the pre-script may now request a skip via the FULLSEND_PRESCRIPT_OUTPUT protocol, causing the run to exit 0 before sandbox creation. The PR already updated docs/architecture.md and docs/guides/dev/cli-internals.md; the ADR is a point-in-time record, so this is a minor gap.

  • [github-action-contract] action.yml — Two new action outputs (skipped and skip-reason) are added to the composite action. This is a backward-compatible, additive change — existing consumers are unaffected. The three-state contract (true/false/empty) is well-documented in the new normative spec.

  • [internal-api-contract] internal/statuscomment/statuscomment.goPostCompletion method signature preserved; now delegates to PostCompletionWithDetail. No breaking change to API surface.

Previous run (2)

Review

Findings

Low

  • [test-naming] internal/cli/prescript_run_test.go:83 — Comment references useFakeOpenshell but the actual helper function is named usePreScriptStub. Appears to be a copy-paste artifact from the existing useFakeOpenshell pattern in run_test.go.

  • [incomplete-documentation] docs/ADRs/0024-harness-definitions.md:110 — The run lifecycle diagram does not mention that the pre-script may now request a skip via the FULLSEND_PRESCRIPT_OUTPUT protocol, causing the run to exit 0 before sandbox creation. The PR already updated docs/architecture.md and docs/guides/dev/cli-internals.md; the ADR is a point-in-time record, so this is a minor gap.

  • [github-action-contract] action.yml — Two new action outputs (skipped and skip-reason) are added to the composite action. This is a backward-compatible, additive change — existing consumers are unaffected. The three-state contract (true/false/empty) is well-documented in the new normative spec.

  • [edge-case] internal/prescript/prescript.go:150bufio.Scanner splits on \n only; a file using bare CR line terminators would be treated as a single long line. The normative doc's characterization of \r as a line terminator refers to the GITHUB_OUTPUT injection risk in values, not the parser's own line splitting. Bare CR-only line endings are not generated by any modern tool.

  • [edge-case] internal/prescript/prescript.go:146 — The scanner buffer maximum (1 MiB) matches the file size cap from the stat check, so the error-message difference for a too-long line is cosmetic at most.


Labels: PR adds pre-script skip protocol to the harness run lifecycle

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge component/harness Agent harness, config, and skills loading labels Jul 29, 2026
@waynesun09 waynesun09 changed the title feat(#4718): add pre-script skip protocol to fullsend run feat(#4718): add pre-script output protocol to fullsend run Jul 29, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 7:36 PM UTC · Ended 7:47 PM UTC
Commit: 0d3332a · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:48 PM UTC · Completed 8:05 PM UTC
Commit: b5c2726 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

Pre-scripts can now request that the run stop before sandbox creation by
writing skipped=true (plus an optional reason=...) to the file named by
the new FULLSEND_PRESCRIPT_OUTPUT env var. fullsend run parses the file
after the script exits, reports a skipped completion status carrying the
reason, relays the outputs to GITHUB_OUTPUT when running under GitHub
Actions, and exits 0.

This is the CLI half of the direction agreed on #4718: with skip gating
living in the CLI, the reusable workflows' inline pre-script calls (the
source of the double execution, and the last consumer of the scaffold
script copies — #5667) become removable, and every forge inherits the
gating without CI-side reimplementation.

Version skew is handled asymmetrically, deliberately: a script that
never writes the file proceeds exactly as before (fails safe), while a
protocol-aware script under an older CLI finds the env var unset and
cannot signal a skip, so the agent runs anyway (fails open). Scripts
must guard on the variable, the same pattern as the existing
GITHUB_OUTPUT guard. Malformed output is a hard error so a mistyped skip
cannot silently proceed as a duplicate agent run — including values that
differ from a reserved key only by case, and values carrying control
characters that could smuggle extra GITHUB_OUTPUT entries.

The normative contract lives in docs/normative/prescript-output/v1.

Assisted-by: Claude (fix, review), Grok (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
The PR mixed three names for the same thing — skip protocol,
FULLSEND_PRESCRIPT_OUTPUT protocol, and contract. Standardize on
"pre-script output protocol" (matching docs/normative/prescript-output/)
as the proper name, with "contract" kept only as a generic descriptor
in the normative doc's opening line. No behavior change.

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
@waynesun09
waynesun09 force-pushed the 4718-prescript-skip-protocol branch from b5c2726 to d08da49 Compare July 29, 2026 20:46
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:47 PM UTC · Completed 9:03 PM UTC
Commit: d08da49 · View workflow run →

@ralphbean ralphbean left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@waynesun09
waynesun09 added this pull request to the merge queue Jul 29, 2026
Merged via the queue into main with commit 306acf0 Jul 29, 2026
24 of 27 checks passed
@waynesun09
waynesun09 deleted the 4718-prescript-skip-protocol branch July 29, 2026 21:28
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 9:31 PM UTC · Completed 9:38 PM UTC
Commit: d08da49 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #5737 — Pre-script skip protocol

Workflow went reasonably well. A human-authored PR (waynesun09) adding a pre-script output protocol to fullsend run — 1,399 additions across 12 files, well-tested and well-documented. The review agent caught a genuine copy-paste artifact the author fixed. Merged in ~3.5 hours with 2 human approvals.

Timeline

  1. 17:57 UTC — PR opened (commit 2aa519f)
  2. 17:58–18:00 — First review run (30477699182) failed — sandbox creation error (crun: unknown version specified: OCI runtime error with openshell 0.0.83)
  3. 18:33–19:06 — Author force-pushed 3× in quick succession; 3 review runs dispatched and cancelled by cancel-in-progress concurrency (30480458046, 30481686310, 30482297780)
  4. 19:08–19:22 — First successful review (30482952073): 3 low findings — 1 true positive (copy-paste naming error), 2 edge-case observations
  5. 19:36–19:47 — Another force push → another cancelled review (30485052363)
  6. 19:48–20:05 — Second successful review (30485918001): same 2 edge-case findings persisted despite author's safe-by-design explanations; inline comments failed to post (GitHub 422 from force-push line mismatch), fell back to body text
  7. 20:18 — ggallen approved; 20:46 — author's final force push with fix
  8. 20:47–21:03 — Third successful review (30489754720): same 2 edge-case findings again, approved
  9. 21:13 — ralphbean approved ("LGTM!"); 21:28 — merged

Key metrics

Metric Value
Review dispatches 8
Completed successfully 3 (37.5%)
Failed (infra) 1
Cancelled (superseded) 4
Wasted compute ~43 min (~48% of total)
True positive findings 1 (copy-paste artifact, fixed)
False positives 2 (edge cases, safe by design)
Human findings agent missed 0

Existing issues with new evidence

What went well

  • The review agent's one true positive (copy-paste naming error in test comment) was genuinely useful — it caught something both human reviewers missed.
  • The 422 inline-comment fallback worked correctly: when force pushes invalidated line positions, the agent degraded gracefully to body-text comments.
  • Human reviewers (ggallen, ralphbean) found no issues the agent missed, suggesting adequate coverage for this PR type.
  • The overall time-to-merge (3.5 hours) was reasonable for a 1,400-line feature PR.

waynesun09 added a commit that referenced this pull request Jul 29, 2026
The skip-flag convention this ADR originally recorded was withdrawn
after review (#5013 and agents#175 closed as superseded). Rewrite the
ADR — still unmerged, so immutability does not apply — to record the
decision that replaced it: pre-scripts run exactly once, inside
fullsend run, signalling skips via the pre-script output protocol
shipped in #5737, with the field-level contract in
docs/normative/prescript-output/v1.

Addresses the review feedback on this PR: options are now one
paragraph each with the decision and its reasons in the Decision
section; the gating no longer lives in GH workflow YAML at all; and no
variables pass between invocations, dissolving the portability concern
rather than specifying a mechanism for it. The non-template References
section is folded into inline links.

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/harness Agent harness, config, and skills loading ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sync scaffold workflow scripts with fullsend-ai/agents copies

3 participants