-
Notifications
You must be signed in to change notification settings - Fork 91
fix(#6798): preserve child top-level fields over inherited forge values #6801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
815cd30
7042c29
728bffb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -629,6 +629,242 @@ model: opus | |
| assert.Equal(t, "gl-pre.sh", h.PreScript) | ||
| } | ||
|
|
||
| func TestLoadWithBase_ChildTopLevelOverridesInheritedForge(t *testing.T) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] naming-convention Subtest names use snake_case (e.g., 'partial_scalars', 'child_has_forge_for_one_platform') while existing subtests in compose_test.go use space-separated lower-case strings (e.g., 'cache hit', 'no overlap appends', 'child overrides base by basename'). Suggested fix: Rename subtests to use spaces instead of underscores for consistency. |
||
| t.Run("scalars", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| writeTestHarness(t, dir, "base.yaml", ` | ||
| agent: agents/test.md | ||
| role: test | ||
| pre_script: base-top-pre.sh | ||
| post_script: base-top-post.sh | ||
| forge: | ||
| github: | ||
| pre_script: base-forge-pre.sh | ||
| post_script: base-forge-post.sh | ||
| policy: base-forge-policy.yaml | ||
| gitlab: | ||
| pre_script: base-forge-gl-pre.sh | ||
| post_script: base-forge-gl-post.sh | ||
| `) | ||
|
|
||
| // Child sets top-level scripts but has no forge block. | ||
| // Child's top-level values must survive ResolveForge. | ||
| path := writeTestHarness(t, dir, "child.yaml", ` | ||
| base: base.yaml | ||
| pre_script: child-pre.sh | ||
| post_script: child-post.sh | ||
| `) | ||
|
|
||
| h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ | ||
| ForgePlatform: "github", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, "child-pre.sh", h.PreScript, "child top-level pre_script must survive inherited forge") | ||
| assert.Equal(t, "child-post.sh", h.PostScript, "child top-level post_script must survive inherited forge") | ||
| // Policy was not set by child, so inherited forge value should apply. | ||
| assert.Equal(t, "base-forge-policy.yaml", h.Policy, "inherited forge policy should apply when child did not set it") | ||
| }) | ||
|
|
||
| t.Run("partial scalars", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| writeTestHarness(t, dir, "base.yaml", ` | ||
| agent: agents/test.md | ||
| role: test | ||
| forge: | ||
| github: | ||
| pre_script: base-forge-pre.sh | ||
| post_script: base-forge-post.sh | ||
| `) | ||
|
|
||
| // Child sets only pre_script but not post_script. | ||
| path := writeTestHarness(t, dir, "child.yaml", ` | ||
| base: base.yaml | ||
| pre_script: child-pre.sh | ||
| `) | ||
|
|
||
| h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ | ||
| ForgePlatform: "github", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, "child-pre.sh", h.PreScript, "child top-level pre_script must win") | ||
| assert.Equal(t, "base-forge-post.sh", h.PostScript, "inherited forge post_script should apply when child did not set it") | ||
| }) | ||
|
|
||
| t.Run("child has forge for one platform", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| writeTestHarness(t, dir, "base.yaml", ` | ||
| agent: agents/test.md | ||
| role: test | ||
| forge: | ||
| github: | ||
| pre_script: base-forge-gh-pre.sh | ||
| gitlab: | ||
| pre_script: base-forge-gl-pre.sh | ||
| `) | ||
|
|
||
| // Child has a forge block for github but not gitlab. | ||
| // Only gitlab should have inherited fields cleared. | ||
| path := writeTestHarness(t, dir, "child.yaml", ` | ||
| base: base.yaml | ||
| pre_script: child-pre.sh | ||
| forge: | ||
| github: | ||
| post_script: child-forge-gh-post.sh | ||
| `) | ||
|
|
||
| h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ | ||
| ForgePlatform: "gitlab", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // GitLab was inherited; child's top-level pre_script should win. | ||
| assert.Equal(t, "child-pre.sh", h.PreScript, "child top-level pre_script must survive inherited gitlab forge") | ||
| }) | ||
|
|
||
| t.Run("skills and providers", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| writeTestHarness(t, dir, "base.yaml", ` | ||
| agent: agents/test.md | ||
| role: test | ||
| forge: | ||
| github: | ||
| skills: | ||
| - base-forge-skill | ||
| providers: | ||
| - base-forge-provider | ||
| `) | ||
|
|
||
| // Child sets top-level skills and providers. | ||
| path := writeTestHarness(t, dir, "child.yaml", ` | ||
| base: base.yaml | ||
| skills: | ||
| - child-skill | ||
| providers: | ||
| - child-provider | ||
| `) | ||
|
|
||
| h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ | ||
| ForgePlatform: "github", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Inherited forge skills/providers should not be concatenated onto child's. | ||
| assert.Equal(t, []string{"child-skill"}, SkillSources(h.Skills), | ||
| "child top-level skills must not be polluted by inherited forge skills") | ||
| assert.Equal(t, []string{"child-provider"}, h.Providers, | ||
| "child top-level providers must not be polluted by inherited forge providers") | ||
| }) | ||
|
|
||
| t.Run("runner env", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| writeTestHarness(t, dir, "base.yaml", ` | ||
| agent: agents/test.md | ||
| role: test | ||
| forge: | ||
| github: | ||
| runner_env: | ||
| SHARED_KEY: base-forge-value | ||
| FORGE_ONLY: forge-only-value | ||
| `) | ||
|
|
||
| // Child sets a matching top-level runner_env key. | ||
| path := writeTestHarness(t, dir, "child.yaml", ` | ||
| base: base.yaml | ||
| runner_env: | ||
| SHARED_KEY: child-value | ||
| `) | ||
|
|
||
| h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ | ||
| ForgePlatform: "github", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, "child-value", h.RunnerEnv["SHARED_KEY"], | ||
| "child top-level runner_env key must survive inherited forge override") | ||
| assert.Equal(t, "forge-only-value", h.RunnerEnv["FORGE_ONLY"], | ||
| "inherited forge env key not set by child should still apply") | ||
| }) | ||
|
|
||
| t.Run("validation loop", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| writeTestHarness(t, dir, "base.yaml", ` | ||
| agent: agents/test.md | ||
| role: test | ||
| forge: | ||
| github: | ||
| validation_loop: | ||
| script: base-forge-validate.sh | ||
| max_iterations: 3 | ||
| `) | ||
|
|
||
| // Child sets a top-level validation_loop. | ||
| path := writeTestHarness(t, dir, "child.yaml", ` | ||
| base: base.yaml | ||
| validation_loop: | ||
| script: child-validate.sh | ||
| max_iterations: 5 | ||
| `) | ||
|
|
||
| h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ | ||
| ForgePlatform: "github", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| require.NotNil(t, h.ValidationLoop, "validation_loop must not be nil") | ||
| assert.Equal(t, "child-validate.sh", h.ValidationLoop.Script, | ||
| "child top-level validation_loop must survive inherited forge override") | ||
| assert.Equal(t, 5, h.ValidationLoop.MaxIterations, | ||
| "child top-level validation_loop max_iterations must survive inherited forge override") | ||
| }) | ||
|
|
||
| t.Run("env sub maps", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| writeTestHarness(t, dir, "base.yaml", ` | ||
| agent: agents/test.md | ||
| role: test | ||
| forge: | ||
| github: | ||
| env: | ||
| runner: | ||
| SHARED_KEY: base-forge-runner-val | ||
| FORGE_ONLY: forge-runner-val | ||
| sandbox: | ||
| SB_SHARED: base-forge-sb-val | ||
| `) | ||
|
|
||
| // Child sets top-level env with a matching key. | ||
| path := writeTestHarness(t, dir, "child.yaml", ` | ||
| base: base.yaml | ||
| env: | ||
| runner: | ||
| SHARED_KEY: child-runner-val | ||
| sandbox: | ||
| SB_SHARED: child-sb-val | ||
| `) | ||
|
|
||
| h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ | ||
| ForgePlatform: "github", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, "child-runner-val", h.Env.Runner["SHARED_KEY"], | ||
| "child top-level env.runner key must survive inherited forge override") | ||
| assert.Equal(t, "forge-runner-val", h.Env.Runner["FORGE_ONLY"], | ||
| "inherited forge env.runner key not set by child should still apply") | ||
| assert.Equal(t, "child-sb-val", h.Env.Sandbox["SB_SHARED"], | ||
| "child top-level env.sandbox key must survive inherited forge override") | ||
| }) | ||
| } | ||
|
|
||
| func TestLoadWithBase_URLBase(t *testing.T) { | ||
| baseContent := []byte(` | ||
| agent: agents/remote.md | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[medium] missing-field
The fix clears inherited forge-level fields (PreScript, PostScript, Policy, Skills, Providers, OpenShell.Profiles, HostFiles, RunnerEnv, Env) on inherited platforms, but omits ValidationLoop. mergeForgeConfig in forge.go (line 387-389) replaces the top-level ValidationLoop entirely when the forge config's is non-nil (h.ValidationLoop = fc.ValidationLoop). If a child harness explicitly sets a top-level validation_loop and extends a base whose forge block also defines one, the inherited forge validation_loop will override the child's during ResolveForge — the same class of bug this PR fixes for other fields.
Suggested fix: Add childHasValidationLoop := child.ValidationLoop != nil to the snapshot block, and if childHasValidationLoop { fc.ValidationLoop = nil } to the clearing loop.