docs(#4718): add ADR 0072 for the pre-script skip-flag convention#5016
docs(#4718): add ADR 0072 for the pre-script skip-flag convention#5016waynesun09 wants to merge 2 commits into
Conversation
|
🤖 Finished Review · ✅ Success · Started 1:59 PM UTC · Completed 2:09 PM UTC |
PR Summary by QodoDocument pre-script skip-flag convention via ADR 0072
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. ADR 0072 lacks context links
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ## Context | ||
|
|
||
| An agent's harness `pre_script` runs once, immediately before sandbox creation, inside `fullsend run`. Some reusable workflows also need a fast pre-check *before* that point, to gate expensive setup (GCP credentials, bot identity, agent-env prep) that would otherwise run unconditionally on invalid input or a redundant retry. `reusable-code.yml` and `reusable-fix.yml` both do this today by calling the same pre-script inline, ahead of `fullsend run` — which means the script runs twice per invocation. | ||
|
|
||
| Deleting the inline call is not always an option: for `code`, the inline step's `skipped=` output gates four downstream workflow steps that all run *before* `fullsend run` starts, so the check has to happen there. Running the pre-script's full body twice is wasteful at best (redundant tool installs) and unsafe at worst (repeated GitHub API side effects — label creation, issue comments — from the existing-human-PR check) ([fullsend-ai/fullsend#4718](https://github.com/fullsend-ai/fullsend/issues/4718)). | ||
|
|
||
| ## Options | ||
|
|
||
| **Split into two scripts** (a lightweight inline "gate" script plus a harness-only "prepare" script) avoids a control flag and keeps each script single-purpose, but requires two entry points per agent to keep in sync and a bigger diff to introduce. **Env var skip-flag** reuses the single existing script and existing delivery mechanisms (`runner_env` / workflow step `env:`), at the cost of a conditional block inside the script. Chose the flag: smaller surface area, and it follows precedent — [ADR 0049](0049-agent-configuration-env-var-convention.md) already establishes `{AGENT}_{SETTING_NAME}` env vars as the standard way to signal harness-scoped behavior differences. |
There was a problem hiding this comment.
1. Adr 0072 lacks context links 📜 Skill insight ⚙ Maintainability
docs/ADRs/0072-pre-script-skip-flag-convention.md has a long Context section that does not link to any docs/problems/* documents (despite relates_to listing agent-architecture and agent-infrastructure) and does not cross-reference related ADRs in Context. This makes the ADR harder to audit and violates the ADR context cross-referencing expectations.
Agent Prompt
## Issue description
ADR 0072's `## Context` section does not link to the relevant problem docs and does not include an explicit cross-reference to related ADRs, even though the ADR declares `relates_to` entries and relies on ADR 0049.
## Issue Context
- The ADR frontmatter lists `relates_to: agent-architecture, agent-infrastructure`, which correspond to existing problem docs under `docs/problems/`.
- The ADR references ADR 0049 elsewhere, but the compliance rule requires related ADR cross-references in the Context section.
## Fix Focus Areas
- docs/ADRs/0072-pre-script-skip-flag-convention.md[22-30]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Review of #5016 — ADR 0072: pre-script skip-flag conventionVerdict: approve · 1 low finding SummaryClean docs-only ADR recording the What was checked
Prior review finding status
Findings1. Context section overstates which gated steps run before
|
| Dimension | Result |
|---|---|
| Correctness | ✅ Internal logic is consistent. The {AGENT}_SKIP_{THING} naming genuinely follows ADR 0049's {AGENT}_{SETTING_NAME} syntax. All referenced ADRs (0024, 0031, 0045, 0049) exist. The code/fix examples correctly illustrate the "whichever invocation the pipeline depends on keeps default behavior" principle. |
| Security | ✅ No findings — clean docs-only change. No secrets, injection vectors, or permission changes. |
| Intent & coherence | ✅ Traces to #4718. Scope matches: one new ADR + one architecture.md bullet. ADR number 0072 is correct (0069 is highest on main; 0070–0071 claimed by open PRs). Status is Accepted (correct for a new ADR per AGENTS.md). relates_to references valid problem docs. PR title follows COMMITS.md (docs type). |
| Style & conventions | ✅ Frontmatter structure, section ordering, and relative link format match recent peer ADRs (0067–0069). The architecture.md bullet matches surrounding formatting (no period before the ADR parenthetical, consistent with all other bullets in the section). |
| Docs currency | ✅ architecture.md updated in the correct section (Agent Harness > Decided). VitePress ADRs sidebar uses getMarkdownFiles() — auto-discovered, no config change needed. |
| Cross-repo contracts | ⏭ Skipped — no APIs, schemas, or exported interfaces modified. |
Findings
1. Missing ADR 0055 reference for env.runner delivery mechanism · low
File: docs/ADRs/0072-pre-script-skip-flag-convention.md (References section)
The References section credits ADR 0024 and ADR 0045 for "runner_env / env.runner delivery mechanism," but env.runner was introduced by ADR 0055 (which deprecated runner_env). ADR 0024 defines runner_env; ADR 0045 defines the forge-portable schema — neither defines env.runner.
The architecture.md bullet for ADR 0055 (lines 112–116) sits directly above the new ADR 0072 bullet, so readers navigating from architecture.md have the deprecation context. However, readers arriving at the ADR directly would benefit from the reference.
Suggested follow-up: Add [ADR 0055](0055-unified-env-var-delivery.md) to the References line, e.g.:
- [ADR 0024](...), [ADR 0045](...), [ADR 0055](...) — `runner_env` (deprecated) / `env.runner` delivery mechanism
This is not blocking — the surrounding context in architecture.md and ADR 0049 already clarifies the deprecation.
Reviewed: 2026-07-15 · Dimensions: correctness (opus), security (opus), intent (sonnet), style (sonnet), docs (sonnet) · Challenger pass: opus
Labels: PR adds a new ADR documenting a harness pre-script convention and updates docs/architecture.md
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
Two fixes from ADR review of #5016: - Cross-reference ADR 0055 (unified env var delivery, deprecates runner_env in favor of env.runner/env.sandbox) instead of naming runner_env and env.runner as interchangeable options with no guidance on which to prefer. - Correct a Consequences bullet that implied removing the inline call only needs fullsend run to surface pre-script outputs earlier — Context already establishes the steps it gates run in an earlier workflow step position, so that alone wouldn't be sufficient; actually reordering those steps would be required too. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Records the pattern introduced in #5013/fullsend-ai/agents#175 so the next agent that needs to gate expensive workflow setup behind a fast pre-check (per rh-hemartin's request for consistency across agents, one: an {AGENT}_SKIP_{THING} env var, set on whichever of the two pre-script call sites should skip the redundant/expensive part. Distinguishes this from ADR 0049's agent-configuration env vars — same {AGENT}_{SETTING} syntax, but an internal invocation-context signal rather than a user-facing behavioral knob, so it belongs in script/harness comments, not docs/agents/<agent>.md. Scopes the decision to agents that actually need workflow-level gating (code, fix today) rather than mandating it retroactively for agents that don't currently duplicate pre-script execution. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Two fixes from ADR review of #5016: - Cross-reference ADR 0055 (unified env var delivery, deprecates runner_env in favor of env.runner/env.sandbox) instead of naming runner_env and env.runner as interchangeable options with no guidance on which to prefer. - Correct a Consequences bullet that implied removing the inline call only needs fullsend run to surface pre-script outputs earlier — Context already establishes the steps it gates run in an earlier workflow step position, so that alone wouldn't be sufficient; actually reordering those steps would be required too. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
69151a9 to
92318d7
Compare
|
🤖 Review · ❌ Terminated · Started 9:40 PM UTC · Ended 9:50 PM UTC |
Site previewPreview: https://0aac9916-site.fullsend-ai.workers.dev Commit: |
|
|
||
| ## Status | ||
|
|
||
| Accepted |
There was a problem hiding this comment.
[low] factual-inaccuracy
Context section states the inline step gates four downstream workflow steps that all run before fullsend run starts. In reusable-code.yml, the fourth gated step (Run code agent, line 201) IS fullsend run — only three of the four steps run before it. The conclusion (inline call cannot be removed) remains correct.
Suggested fix: Rephrase to: gates four downstream workflow steps — three setup steps and the fullsend run invocation itself — so the check has to happen there.
fullsend-ai-review
Bot
added
ready-for-merge
| ## Context | ||
|
|
||
| An agent's harness `pre_script` runs once, immediately before sandbox creation, inside `fullsend run`. Some reusable workflows also need a fast pre-check *before* that point, to gate expensive setup (GCP credentials, bot identity, agent-env prep) that would otherwise run unconditionally on invalid input or a redundant retry. `reusable-code.yml` and `reusable-fix.yml` both do this today by calling the same pre-script inline, ahead of `fullsend run` — which means the script runs twice per invocation. | ||
|
|
There was a problem hiding this comment.
The current approach seems to put too much functionality into the GH workflow - this makes it harder to port to other platforms or into the generic "harness-trigger" flow.
We need to consider moving all the dowstream steps into the pre script itself or otherwise into functionality in fullsend run.
|
|
||
| ## Options | ||
|
|
||
| **Split into two scripts** (a lightweight inline "gate" script plus a harness-only "prepare" script) avoids a control flag and keeps each script single-purpose, but requires two entry points per agent to keep in sync and a bigger diff to introduce. **Env var skip-flag** reuses the single existing script and existing delivery mechanisms (`runner_env` / workflow step `env:`), at the cost of a conditional block inside the script. Chose the flag: smaller surface area, and it follows precedent — [ADR 0049](0049-agent-configuration-env-var-convention.md) already establishes `{AGENT}_{SETTING_NAME}` env vars as the standard way to signal harness-scoped behavior differences. |
There was a problem hiding this comment.
this is mixing all the options and decisions into a single dense paragraph, break it apart please - give each option its own paragraph and put the decision and the reasons for it in the decision section where it belongs.
|
|
||
| When a pre-script must run in two contexts that need different subsets of its behavior — an inline reusable-workflow step needing only a fast, gating subset, and the harness `pre_script` invocation needing the full script — gate the part that must run exactly once behind a dedicated env var, rather than deleting either call site. | ||
|
|
||
| **Naming:** `{AGENT}_SKIP_{THING}`, following [ADR 0049](0049-agent-configuration-env-var-convention.md)'s `{AGENT}_{SETTING_NAME}` syntax. Examples: `CODE_SKIP_EXISTING_PR_CHECK`, `FIX_SKIP_TOOL_INSTALL`. |
There was a problem hiding this comment.
If, while considering environment portability, we still decide that we want to run the script twice and pass varaibles between the two times, then we need to specify the mechanism by which the variables are set (hard-coding this in the workflow YAML is not portable).
2 participants
Summary
Records the
{AGENT}_SKIP_{THING}skip-flag convention introduced by #5013 / fullsend-ai/agents#175, so it's a documented pattern rather than something reviewed from scratch each time an agent needs it.Related Issue
Relates to #4718 — specifically rh-hemartin's request that every agent gate expensive setup consistently, not just code/fix.
Why now
rh-hemartin pushed back on the env-var mechanism in #4762 before agreeing it was the right approach, and separately asked for it to be applied consistently across agents. Rather than re-litigate the mechanism on every future PR that needs it, this ADR records the decision once: what the flag naming is, which invocation sets it, why it's a different category from ADR 0049's user-facing config vars, and the scope (apply when an agent actually needs workflow-level gating — not retroactively to agents that don't).
Changes
docs/ADRs/0072-pre-script-skip-flag-convention.md— new ADR (Accepted)docs/architecture.md— one-bullet addition under "Agent Harness > Decided:", linking the new ADR (perskills/writing-adrs/SKILL.md's living-document update step)Testing
make lintpasses (staged), includinglint ADR statuses,lint ADR numbers,lint ADR frontmatter,lint markdown linksskills/writing-adrs/SKILL.mdchecklist: scanneddocs/ADRs/onmain+ open PRs for in-flight numbers (0070 and 0071 both already claimed by other open PRs — used 0072), used the template, matched frontmatterstatusto the## Statusheading, no leading zeros in the title number