Skip to content

feat(#5970): add providers and openshell to ForgeConfig - #6139

Merged
ggallen merged 1 commit into
mainfrom
agent/5970-forge-providers-openshell
Aug 12, 2026
Merged

feat(#5970): add providers and openshell to ForgeConfig#6139
ggallen merged 1 commit into
mainfrom
agent/5970-forge-providers-openshell

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

  • Add providers and openshell fields to ForgeConfig struct, enabling per-forge provider and profile selection in a single portable harness
  • Extend mergeForgeConfig (forge resolution) and mergeForgeConfigInto (base composition) with concatenation logic for the new fields, following the same merge rules as skills per ADR-0045
  • Extend validateForge to validate URL integrity hashes on forge-specific provider and profile entries
  • Extend resolveBaseProviders and resolveBaseProfiles to fetch and cache forge-level entries from URL-referenced base harnesses

Context

This follows the same precedent as #5858 (adding policy to ForgeConfig) and implements the full harness composition checklist from docs/contributing/harness-composition.md. All paired merge functions (mergeForgeConfig, mergeForgeConfigInto, mergeForgeBlocks) are kept in sync.

Testing

  • Added tests for mergeForgeConfig (ResolveForge): providers concatenation, openshell concatenation, nil inheritance, nil top-level
  • Added tests for mergeForgeConfigInto: providers inherited/concatenated, openshell inherited/concatenated
  • Added tests for validateForge: provider URL without/with hash, openshell profile URL without/with hash
  • Added YAML parsing test for forge-level providers and openshell
  • All existing tests pass (go test ./internal/harness/...)

Caveats

  • pre-commit could not run in the sandbox (network restrictions); the post-script runs it authoritatively
  • golangci-lint was not available in the sandbox

Closes #5970

Post-script verification

  • Branch is not main/master (agent/5970-forge-providers-openshell)
  • Secret scan passed (gitleaks — 55bd9f61ce57f9f2151a29d59a04d42b664110cf..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner August 12, 2026 10:52
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Aug 12, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:53 AM UTC · Completed 11:13 AM UTC

Commit: cb5f845 · View workflow run →

@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.36842% with 12 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/harness/compose.go 85.29% 5 Missing and 5 partials ⚠️
internal/cli/lock.go 66.66% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

Looks good to me

Previous run

Review

Findings

Low

  • [stale-field-reference] docs/architecture.md:108 — The architecture document describes forge-portable harness schema but lists only scripts, skills, runner_env as forge-level fields in the parenthetical on line 108. It does not mention the newly added providers and openshell fields that can now appear at the forge level per this PR's changes. The parenthetical list was already non-exhaustive (it also omits host_files, validation_loop, policy, and env), so this is a pre-existing gap widened by this PR.
    Remediation: Update line 108 to either include providers and openshell in the parenthetical list, or replace the enumeration with a forward reference to ADR-0045's authoritative table (e.g., 'platform-specific config (see ADR-0045 for the full list)').
Previous run (2)

Review

Findings

Low

  • [stale-field-reference] docs/architecture.md:108 — The architecture document describes forge-portable harness schema but lists only scripts, skills, runner_env as forge-level fields. It does not mention the newly added providers and openshell fields that can now appear at the forge level per this PR's changes. The parenthetical list was already non-exhaustive (it also omits host_files, validation_loop, policy, and env), so this is a pre-existing gap widened by this PR.
    Remediation: Update line 108 to include providers and openshell in the forge-level field list.
Previous run (3)

Looks good to me

Previous run (4)

Review

Findings

Medium

  • [consumer-completeness] internal/cli/lock.go:961 — The PR introduces forge-level dependency field names forge.<platform>.providers[N] and forge.<platform>.openshell.profiles[N] (generated by resolveBaseProviders and resolveBaseProfiles in compose.go), but the resolveFromLock mutation switch in lock.go has no cases for these patterns. They fall through to the default case, which tries Sscanf("skills[%d]"), fails, and unconditionally appends the cache path to h.Skills. This silently corrupts the harness at runtime when a lock file contains forge-level provider or profile entries. The existing forge.*.skills[*] handler demonstrates the expected pattern.
    Remediation: Add two no-op cases in the resolveFromLock mutation switch — one for forge-scoped providers (forge. prefix + .providers[ contains) and one for forge-scoped profiles (forge. prefix + .openshell.profiles[ contains) — analogous to the existing forge.*.skills[*] handler.

Low

  • [consumer-completeness] internal/cli/lock.go:827 — The resolveFromLock profile/provider reconstruction section uses strings.HasPrefix checks that don't match forge-scoped field names like forge.github.providers[0]. Forge-scoped lock deps won't get ResolvedProfile/ResolvedProvider reconstruction, potentially missing the .yaml named symlink step.
    Remediation: Extend the prefix checks to also match forge-scoped entries, or add comments explaining why forge-level entries are intentionally excluded from reconstruction.

  • [stale-field-list] docs/guides/user/bring-your-own-agent.md:191 — The description of forge.github says it "scopes scripts, skills, host_files, and env vars to GitHub" but providers and openshell are now also forge-scoped.
    Remediation: Update the description to include providers and openshell in the list of forge-scoped fields.

  • [stale-merge-table] docs/guides/user/bring-your-own-agent.md:316 — The field merge rules table groups plugins, providers, api_servers, openshell.profiles into a single row but doesn't distinguish fields that participate in forge-level merging from those that don't.
    Remediation: Split the row to differentiate forge-applicable fields from top-level-only fields.

  • [incomplete-example] docs/guides/user/bring-your-own-agent.md:276 — The forge block YAML example omits providers and openshell as possible forge-level fields.
    Remediation: Consider adding providers and/or openshell entries to the forge block example.

  • [stale-glossary-entry] docs/glossary.md:35 — The Base Composition glossary entry omits openshell.profiles from the concatenated fields list.
    Remediation: Update the glossary entry to include openshell.profiles.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (5)

Review

Findings

High

  • [architectural-coherence] docs/ADRs/0045-forge-portable-harness-schema.md:262 — ADR-0045 explicitly classifies providers as forge-agnostic in the "Fields that stay at top level only (platform-neutral)" table: "providers | OpenShell providers are forge-agnostic." This PR adds Providers and OpenShell to ForgeConfig, making them forge-specific, which directly contradicts the ADR's architectural classification. The ForgeConfig struct definition in the ADR (~line 471) is also missing the new fields. While issue harness: add providers and openshell to ForgeConfig for per-forge provider/profile selection #5970 correctly identifies the need for per-forge provider/profile selection, the ADR must be updated to reflect this architectural change.
    Remediation: Update ADR-0045: (1) move providers from the "Fields that stay at top level only" table to the "Fields that can appear at both levels" table with rationale; (2) add openshell to the forge-overridable table; (3) update the ForgeConfig struct definition to include Providers []string and OpenShell *OpenShellConfig; (4) add merge rules for providers (concatenated) and openshell.profiles (concatenated) to the inheritance table.

Low

  • [test-adequacy] internal/harness/compose_test.go — The PR adds forge-specific code paths in resolveBaseProfiles and resolveBaseProviders (iterating over base.Forge to fetch and cache forge-level profile/provider files), but no tests exercise these new loops. The early-exit logic change — which prevents forge-only profiles/providers from being silently skipped when no top-level entries exist — is also untested.
    Remediation: Add tests for resolveBaseProfiles and resolveBaseProviders with forge-level entries, including a test where only forge-level (not top-level) entries exist to verify the early-exit fix.

Labels: PR modifies harness composition and forge configuration in internal/harness/


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the component/harness Agent harness, config, and skills loading label Aug 12, 2026
@ggallen
ggallen force-pushed the agent/5970-forge-providers-openshell branch from cb5f845 to 5e02a24 Compare August 12, 2026 11:18
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 11:19 AM UTC · Ended 11:35 AM UTC

Commit: 5e02a24 · View workflow run →

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

Site preview

Preview: https://74381089-site.fullsend-ai.workers.dev

Commit: c202c0b3539bc9a69a85b70636ebe1e502c75a91

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:19 AM UTC · Completed 11:35 AM UTC

Commit: 5e02a24 · View workflow run →

@ggallen
ggallen force-pushed the agent/5970-forge-providers-openshell branch from 5e02a24 to 79c5f8a Compare August 12, 2026 11:40
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:41 AM UTC · Completed 11:58 AM UTC

Commit: 79c5f8a · View workflow run →

@fullsend-ai-review
fullsend-ai-review Bot dismissed stale reviews from themself August 12, 2026 11:58

Superseded by updated review

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Aug 12, 2026
@ggallen
ggallen force-pushed the agent/5970-forge-providers-openshell branch from 79c5f8a to afd133f Compare August 12, 2026 12:04
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:06 PM UTC · Completed 12:21 PM UTC

Commit: afd133f · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@ggallen
ggallen force-pushed the agent/5970-forge-providers-openshell branch from afd133f to 667cd35 Compare August 12, 2026 19:32
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:34 PM UTC · Completed 7:48 PM UTC

Commit: 667cd35 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

ForgeConfig now supports providers and openshell fields, enabling
per-forge provider and profile selection in a single portable
harness. This follows the same merge rules as skills (top-level +
forge, concatenated) per ADR-0045.

Changes across the paired merge functions per the harness
composition checklist:

- ForgeConfig struct (forge.go): add Providers []string and
  OpenShell *OpenShellConfig fields with YAML tags
- mergeForgeConfig (forge.go): concatenate providers and
  openshell.profiles from forge block onto top-level harness
- validateForge (forge.go): validate URL integrity hashes on
  forge-specific provider and profile entries
- mergeForgeConfigInto (compose.go): same concatenation logic
  for base composition of forge blocks
- resolveBaseProviders (compose.go): extend to fetch and cache
  forge-level provider paths from URL-referenced base harnesses
- resolveBaseProfiles (compose.go): extend to fetch and cache
  forge-level profile paths from URL-referenced base harnesses
- ADR-0045: update to reflect providers and openshell as
  forge-overridable fields with concatenation merge rules

Closes #5970

Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Greg Allen <gallen@redhat.com>
@ggallen
ggallen force-pushed the agent/5970-forge-providers-openshell branch from 667cd35 to c202c0b Compare August 12, 2026 19:59
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:00 PM UTC · Completed 8:13 PM UTC

Commit: c202c0b · View workflow run →

@ggallen
ggallen enabled auto-merge August 12, 2026 20:20
@ggallen
ggallen added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit baecf40 Aug 12, 2026
32 of 42 checks passed
@ggallen
ggallen deleted the agent/5970-forge-providers-openshell branch August 12, 2026 21:57
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 12, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 9:59 PM UTC · Completed 10:19 PM UTC

Commit: c202c0b · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #6139feat(#5970): add providers and openshell to ForgeConfig

Timeline

Issue #5970 was triaged on 2026-08-06. On 2026-08-12, ggallen triggered the code agent via /fs-code. The code agent (run 31588582127) produced PR #6139 in ~10 minutes ($4.05, 60 turns). The PR went through 6 review agent runs and 2 failed fix agent dispatches before human approval and merge ~11 hours later.

What went well

Review agent caught two non-trivial bugs:

  • HIGH (run 1): ADR-0045 classifies providers as forge-agnostic, but the PR made them forge-specific — an architectural coherence violation requiring ADR updates.
  • MEDIUM (run 2): resolveFromLock in lock.go had no switch cases for forge-scoped provider/profile field names, which would silently corrupt the harness by appending cache paths to h.Skills at runtime.

Both were genuine finds that a human reviewer could easily miss. The human fixed all findings, and the review agent correctly approved after remediation (run 3). The human reviewer approved with no additional findings — the agent drove all substantive improvements.

What could go better

Fix agent was blocked (evidence for #5536, #6007, #1569). Two fix runs (31590926132, 31592565886) failed the eligibility check because gh pr view --json author (GraphQL) returns bot authors as app/fullsend-ai-coder without the [bot] suffix, so the regex \[bot\]$ doesn't match. The human had to manually fix both the HIGH and MEDIUM findings. This is the single biggest time-saving opportunity — if the fix agent had worked, the 2-hour manual fix cycle (runs 1–3) could have been automated.

Harness-composition.md checklist gap (evidence for #5579). The checklist omits resolveFromLock in internal/cli/lock.go as a consumer that must be updated when new forge-level field names are introduced. The triage agent faithfully reproduced this incomplete checklist, and the code agent followed it — so neither agent caught the lock.go gap. The review agent caught it on run 2. Issue #5579 proposes a broader harness-fields.md that would cover composition carry-forward; this PR provides concrete evidence that the lock.go consumer is a critical omission.

Code agent didn't follow ADR reference (evidence for #3158). The harness-composition.md checklist references ADR-0045 in step 1 and the Related section. The code agent read harness-composition.md but did not follow the ADR link to check field classification tables. Issue #3158 proposes adding an ADR-awareness step to the code-implementation skill's planning phase; this PR is another data point supporting that proposal.

Review finding re-raised across runs (evidence for #5007, #2959). The same LOW finding (stale docs/architecture.md:108 reference) appeared on runs 4 and 5 because the human hadn't fixed it between those pushes. The agent correctly re-flagged it, but existing deduplication issues (#5007, #2959) track ways to reduce noise for findings already present in a prior approval.

Autonomy readiness

The review agent outperformed the human reviewer on this PR: it caught 2 non-trivial bugs (ADR contradiction, silent runtime corruption) that the human did not flag independently. The human approval added zero findings beyond the agent's. The change class (harness field addition) is well-documented with an explicit checklist, making it highly tractable for automated review. This is evidence supporting expanded agent autonomy for internal/harness/ changes following the harness-composition pattern, subject to the methodology caveat in #5156.

Proposals filed

dhshah13 pushed a commit to dhshah13/fullsend that referenced this pull request Aug 20, 2026
The harness-composition.md checklist (steps 1-5) covered merge function
updates but did not include maintaining field classification tables when
a field's forge-level status changed. This caused a HIGH review finding
on PR fullsend-ai#6139 where ADR-0045's tables contradicted the code change.

Per maintainer direction, instead of adding a step that points to
the frozen ADR, create a living reference document
(docs/contributing/harness-fields.md) that extracts the evolving
field classifications, merge rules, and ForgeConfig struct from
ADR-0045 and overlay rules from ADR-0088.

Changes:
- Create docs/contributing/harness-fields.md as the authoritative
  living reference for field classifications and merge rules
- Add step 6 to harness-composition.md checklist pointing to
  the living doc
- Mark ADR-0045 as Superseded by ADR-0088 with cross-reference
  to the living doc
- Add cross-reference note to ADR-0088 pointing to the living doc
- Update docs/architecture.md to note ADR-0045 supersession and
  link to the living doc
- Add harness-fields.md to AGENTS.md topic-specific guidance table

Note: pre-commit could not run (sandbox network policy blocks git
fetch for hook environment setup). The post-script runs pre-commit
authoritatively on the runner.

Closes fullsend-ai#6160
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 ready-for-review Triggers review agent dispatch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

harness: add providers and openshell to ForgeConfig for per-forge provider/profile selection

1 participant