From cb31d066db81b1464fa8a32a0870f05269a21636 Mon Sep 17 00:00:00 2001 From: Daniel Green Date: Thu, 7 May 2026 17:04:42 -0700 Subject: [PATCH] Phase 7 dogfood follow-up: bug #8 (ancestor_chain.parent_item_id always-emit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `polyphony plan derive-ancestor-chain --root-id R --item-id R` (the root-plan case) silently dropped `parent_item_id` from its JSON output because `PolyphonyJsonContext` defaults to `JsonIgnoreCondition.WhenWritingNull`. Conductor renders the next agent's prompt under `strict_undefined`, which raises on attribute access for missing dict keys *before* the workflow's `| default(0)` filter can substitute — so dogfood iter 4 (apex #3043, 2026-05-08) crashed inside `ensure_plan_branch` with: Undefined variable in template: 'dict object' has no attribute 'parent_item_id' Compounding gotcha: even with the field present-as-null, Jinja's bare `default(0)` filter only substitutes on Undefined, NOT on `None`. The two-arg form `default(0, true)` (boolean=True) substitutes on both. Fix is Option A from the state-effects catalog — surgical override at the verb's output model, plus the matching workflow Jinja change: - src/Polyphony/Models/PlanDeriveAncestorChainResult.cs: per-property `[JsonIgnore(Condition = JsonIgnoreCondition.Never)]` on `ParentItemId`. Always serialized as `null` for root + direct children of root, integer for deeper descendants. Class-level XML doc updated; property-level XML doc captures the bug-#8 history and the rationale for the per-property attribute over a global flip. - .conductor/registry/workflows/plan-level.yaml: 5 references to `ancestor_chain.output.parent_item_id | default(0)` updated to the two-arg `default(0, true)` form (lines 754, 859, 1530, 1837, 1964). Regression coverage: - 4 new xUnit wire-shape tests in PlanCommandsDeriveAncestorChainTests.cs pinning the JSON shape on the root-plan, direct-child-of-root, descendant, and error paths. All assert `"parent_item_id":` is present in output. - 2 new Pester regression tests in lint-plan-level.Tests.ps1 (new `parent_item_id default-filter form` Context): - asserts the production yaml uses `default(0, true)` for every `parent_item_id` reference (no bare `default(0)` remains); - asserts the lint fires the `parent-item-id-bare-default` rule when the production yaml is mutated back to the bare form. - New lint check in lint-plan-level.ps1 (Check 25) regex-scans for `parent_item_id\s*\|\s*default\(\s*0\s*\)` and refuses on match. Catalog updates (docs/polyphony-state-effects-catalog.md): - New verb entry: `polyphony plan derive-ancestor-chain` with wire-shape gotcha (`parent_item_id` always emitted; bare `default(0)` semantics). - New gap entry: "DefaultIgnoreCondition.WhenWritingNull breaks strict_undefined Jinja consumers" — flags this as a pattern affecting every nullable verb-output field reachable from a strict_undefined Jinja consumer; calls out the per-property vs context-wide-flip decision as a deferred systematic question. Verification: - Build clean (Release). - xUnit: 2435 passed (was 2431; +4 wire-shape tests). - Pester (lint-plan-level): 25 passed (was 23; +2 regression tests). - All 14 conductor lints PASS. - Type-agnostic lint PASS (48 files / surface=all). - conductor validate PASS for all 14 workflows. - Smoke: `polyphony plan derive-ancestor-chain --root-id 3043 --item-id 3043` now emits `"parent_item_id":null` (previously elided). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../registry/tests/lint-plan-level.Tests.ps1 | 38 +++++++++++ .conductor/registry/tests/lint-plan-level.ps1 | 20 ++++++ .conductor/registry/workflows/plan-level.yaml | 10 +-- docs/polyphony-state-effects-catalog.md | 43 +++++++++++++ .../Models/PlanDeriveAncestorChainResult.cs | 23 ++++++- .../PlanCommandsDeriveAncestorChainTests.cs | 63 +++++++++++++++++++ 6 files changed, 190 insertions(+), 7 deletions(-) diff --git a/.conductor/registry/tests/lint-plan-level.Tests.ps1 b/.conductor/registry/tests/lint-plan-level.Tests.ps1 index eb312c22..2b843631 100644 --- a/.conductor/registry/tests/lint-plan-level.Tests.ps1 +++ b/.conductor/registry/tests/lint-plan-level.Tests.ps1 @@ -856,4 +856,42 @@ agents: ($output -join "`n") | Should -Match 'missing-warning-mode-route' } } + + Context 'parent_item_id default-filter form (regression: dogfood apex #3043, 2026-05-08)' { + + BeforeEach { + $script:TempRoot = Join-Path ([System.IO.Path]::GetTempPath()) "lint-plan-level-pid-$([guid]::NewGuid().ToString('N').Substring(0,8))" + $script:WorkflowsDir = Join-Path $script:TempRoot 'workflows' + $script:TestsDir = Join-Path $script:TempRoot 'tests' + $script:RealYaml = Join-Path $PSScriptRoot '..' 'workflows' 'plan-level.yaml' + New-Item $script:WorkflowsDir -ItemType Directory -Force | Out-Null + New-Item $script:TestsDir -ItemType Directory -Force | Out-Null + Copy-Item $script:LintScript (Join-Path $script:TestsDir 'lint-plan-level.ps1') + } + + AfterEach { + Remove-Item $script:TempRoot -Recurse -Force -ErrorAction SilentlyContinue + } + + It 'Real plan-level.yaml uses the two-arg default(0, true) form for every parent_item_id reference' { + $content = Get-Content $script:RealYaml -Raw + # No bare default(0) form should remain — every reference uses default(0, true). + $bareMatches = [regex]::Matches($content, 'parent_item_id\s*\|\s*default\(\s*0\s*\)') + $bareMatches.Count | Should -Be 0 + # And the two-arg form IS present (sanity check that we have references at all). + $twoArgMatches = [regex]::Matches($content, 'parent_item_id\s*\|\s*default\(\s*0\s*,\s*true\s*\)') + $twoArgMatches.Count | Should -BeGreaterThan 0 + } + + It 'Lint fails when plan-level.yaml regresses to bare default(0)' { + $content = Get-Content $script:RealYaml -Raw + # Mutate every two-arg form back to the bare form. + $mutated = $content -replace 'parent_item_id\s*\|\s*default\(\s*0\s*,\s*true\s*\)', 'parent_item_id | default(0)' + Set-Content (Join-Path $script:WorkflowsDir 'plan-level.yaml') $mutated + $lintScript = Join-Path $script:TestsDir 'lint-plan-level.ps1' + $output = pwsh -NoProfile -File $lintScript 2>&1 + $LASTEXITCODE | Should -Be 1 + ($output -join "`n") | Should -Match 'parent-item-id-bare-default' + } + } } diff --git a/.conductor/registry/tests/lint-plan-level.ps1 b/.conductor/registry/tests/lint-plan-level.ps1 index c915ce9d..89553def 100644 --- a/.conductor/registry/tests/lint-plan-level.ps1 +++ b/.conductor/registry/tests/lint-plan-level.ps1 @@ -347,6 +347,26 @@ if ($policyBlock -match 'type_loader\.output\.type_name\b') { Rule = 'open-questions-policy-bad-type-field' Detail = "open_questions_policy --scope references type_loader.output.type_name; the verb emits 'type', not 'type_name' (caused dogfood failure on apex #3043, 2026-05-07)" } +}# ── Check 25: ancestor_chain.output.parent_item_id uses default(0, true) ─ +# Bug #8 (dogfood apex #3043, 2026-05-08). The verb's wire shape carries +# parent_item_id as JSON null on the root-plan path; Jinja's bare +# `default(0)` filter substitutes only on Undefined, NOT on a present +# null value. The two-arg `default(0, true)` form substitutes on both +# Undefined and falsy (including None). Every reference to +# `ancestor_chain.output.parent_item_id` that wraps in `default()` MUST +# use the two-arg form, or the recursive `for_each` rebuilds with +# parent_item_id=None and the polyphony CLI rejects it. +# +# This regex finds bare `parent_item_id | default(0)` (no second arg). +# The two-arg form `default(0, true)` is allowed. +$badDefaultMatches = [regex]::Matches( + $content, + 'parent_item_id\s*\|\s*default\(\s*0\s*\)') +if ($badDefaultMatches.Count -gt 0) { + $violations += [PSCustomObject]@{ + Rule = 'parent-item-id-bare-default' + Detail = "Found $($badDefaultMatches.Count) reference(s) to ancestor_chain.output.parent_item_id with bare 'default(0)'; must use two-arg form 'default(0, true)' so JSON null is coerced (caused dogfood failure on apex #3043, 2026-05-08)" + } } # ── Report ─────────────────────────────────────────────────────────────── diff --git a/.conductor/registry/workflows/plan-level.yaml b/.conductor/registry/workflows/plan-level.yaml index 94570bcc..94164ebc 100644 --- a/.conductor/registry/workflows/plan-level.yaml +++ b/.conductor/registry/workflows/plan-level.yaml @@ -751,7 +751,7 @@ agents: - "--item-id" - "{{ workflow.input.work_item_id }}" - "--parent-item-id" - - "{{ ancestor_chain.output.parent_item_id | default(0) }}" + - "{{ ancestor_chain.output.parent_item_id | default(0, true) }}" routes: - to: ensure_plan_branch_error_gate when: "{{ ensure_plan_branch.output.error is defined and ensure_plan_branch.output.error }}" @@ -856,7 +856,7 @@ agents: - "--item-id" - "{{ workflow.input.work_item_id }}" - "--parent-item-id" - - "{{ ancestor_chain.output.parent_item_id | default(0) }}" + - "{{ ancestor_chain.output.parent_item_id | default(0, true) }}" - "--ancestor-ids" - "{{ ancestor_chain.output.ancestor_ids }}" routes: @@ -1527,7 +1527,7 @@ agents: - "--item-id" - "{{ workflow.input.work_item_id }}" - "--parent-item-id" - - "{{ ancestor_chain.output.parent_item_id | default(0) }}" + - "{{ ancestor_chain.output.parent_item_id | default(0, true) }}" routes: - to: stale_generation_gate when: "{{ merge_plan_pr.output.error_code is defined and merge_plan_pr.output.error_code == 'stale_generation' }}" @@ -1834,7 +1834,7 @@ agents: - "--item-id" - "{{ workflow.input.work_item_id }}" - "--parent-item-id" - - "{{ ancestor_chain.output.parent_item_id | default(0) }}" + - "{{ ancestor_chain.output.parent_item_id | default(0, true) }}" - "--ancestor-ids" - "{{ ancestor_chain.output.ancestor_ids }}" routes: @@ -1961,7 +1961,7 @@ agents: - "--item-id" - "{{ workflow.input.work_item_id }}" - "--parent-item-id" - - "{{ ancestor_chain.output.parent_item_id | default(0) }}" + - "{{ ancestor_chain.output.parent_item_id | default(0, true) }}" routes: # NOTE: stale_generation reuses stale_generation_gate. The gate's # prompt references merge_plan_pr.output.* fields — those are diff --git a/docs/polyphony-state-effects-catalog.md b/docs/polyphony-state-effects-catalog.md index 3ba1d3dc..3d6a192d 100644 --- a/docs/polyphony-state-effects-catalog.md +++ b/docs/polyphony-state-effects-catalog.md @@ -158,6 +158,31 @@ introduce new dependencies. runtime. Pinned by lint check `open-questions-policy-bad-type-field` in `lint-plan-level.ps1` as of this PR. +### `polyphony plan derive-ancestor-chain --root-id R --item-id I` +- **Purpose**: derive the parent-chain (ancestors from root → leaf) for + an item under a given apex root. Workflows feed the result into + recursive planning to know who the parent plan branch is. +- **Pre**: ADO reachable; `R > 0`, `I > 0`. +- **Post**: emits `{root_id, item_id, is_root_plan, parent_item_id, + ancestor_ids, ancestor_chain, depth}`. On error, `{error}` is added. +- **Side effects**: none (read-only — walks the work-item tree). +- **Idempotent**: yes. +- **Wire-shape gotcha**: `parent_item_id` is `int?` and is **always + emitted** (per-property `[JsonIgnore(Condition = Never)]` overrides the + `PolyphonyJsonContext` default of `WhenWritingNull`). The field is + `null` for the root-plan case (`item_id == root_id`) and for direct + children of root, and an integer for deeper descendants. + + Bug #8 (dogfood apex #3043, 2026-05-08) had the field elided on the + root path; conductor's `strict_undefined` then raised + `'dict object' has no attribute 'parent_item_id'` when the workflow + tried to thread it into the recursive `for_each` step. Even after the + field is always emitted, Jinja's bare `default(0)` filter does NOT + substitute on `None` — only on `Undefined`. Workflow MUST use + `default(0, true)` (two-arg, `boolean=True`) to coerce both. Pinned + by lint check `parent-item-id-bare-default` in `lint-plan-level.ps1` + as of this PR. + ### `polyphony policy resolve --domain --scope [--path P]` - **Purpose**: resolve effective policy for a `(scope, domain)` pair by layering most-specific-wins (`type:Name` > `root` > `defaults`). @@ -267,6 +292,24 @@ introduce new dependencies. agree. A general remedy would be a registry of conductor-honored Jinja functions/filters that lints can validate against — also rolls into [#163](https://github.com/PolyphonyRequiem/polyphony/issues/163). +- **`DefaultIgnoreCondition.WhenWritingNull` breaks `strict_undefined` + Jinja consumers**: `PolyphonyJsonContext` defaults to + `JsonIgnoreCondition.WhenWritingNull`, so any nullable verb-output + field (`int?`, `string?`, etc.) is silently elided when its value is + null. Conductor renders agent prompts with `strict_undefined`, which + raises on attribute access for missing dict keys *before* any + `default()` filter can apply. Bug #8 (dogfood apex #3043, + 2026-05-08) was the first instance: `plan derive-ancestor-chain`'s + `parent_item_id` was elided on the root path, blowing up the + recursive `for_each` consumer. Compounding gotcha: even when the + field IS present-as-null, Jinja's bare `default(0)` only substitutes + on Undefined, NOT on `None` — workflow must use the two-arg form + `default(0, true)` to coerce `None → 0`. Per-property + `[JsonIgnore(Condition = Never)]` is the surgical Option-A fix; the + blanket alternative (flip the context default to `Never` globally) + would tax every error-envelope shape and is deferred as a systematic + decision. **This pattern affects every nullable verb-output field** + reachable from a `strict_undefined` Jinja consumer — audit needed. - **Wave integration idempotency**: not yet exercised; document after first wave-integration smoke. - **`apex-wave-dispatch.yaml` and per-lifecycle sub-workflows**: not diff --git a/src/Polyphony/Models/PlanDeriveAncestorChainResult.cs b/src/Polyphony/Models/PlanDeriveAncestorChainResult.cs index be83c8c7..33cfc594 100644 --- a/src/Polyphony/Models/PlanDeriveAncestorChainResult.cs +++ b/src/Polyphony/Models/PlanDeriveAncestorChainResult.cs @@ -1,5 +1,7 @@ namespace Polyphony; +using System.Text.Json.Serialization; + /// /// Output of polyphony plan derive-ancestor-chain. Walks the work-item /// parent chain from up to (but not past) @@ -8,8 +10,8 @@ namespace Polyphony; /// --root-id for every verb (already known by the workflow). /// --parent-item-id for branch ensure-plan, /// pr open-plan-pr, and pr merge-plan-pr — required for -/// descendants of descendants; omitted (null) for the root plan and -/// direct children of root. +/// descendants of descendants; null for the root plan and direct +/// children of root. /// --ancestor-ids for pr open-plan-pr — comma-separated /// chain (immediate parent first), with the literal "root" /// token used in place of the root work-item id, e.g. "5678,root" @@ -32,7 +34,24 @@ public sealed record PlanDeriveAncestorChainResult /// Immediate plan-tree parent's work-item id, or null when the parent is /// implicit (root plan, or direct child of root). Maps directly to the /// --parent-item-id flag of the plan-PR verbs (omit when null). + /// + /// + /// Always serialized to JSON (overrides the per-context + /// WhenWritingNull default) so workflow Jinja consumers under + /// strict_undefined can reference output.parent_item_id + /// unconditionally without raising on a missing attribute. The wire shape + /// is uniform across root-plan and descendant-plan invocations. + /// + /// + /// Bug #8 (dogfood apex #3043, 2026-05-08) surfaced the original wire + /// shape: WhenWritingNull elided the field for the root case, + /// then conductor's strict_undefined raised on + /// ancestor_chain.output.parent_item_id | default(0) because + /// Jinja's default() filter triggers on Undefined values, not on + /// missing attributes of a defined dict. + /// /// + [JsonIgnore(Condition = JsonIgnoreCondition.Never)] public int? ParentItemId { get; init; } /// diff --git a/tests/Polyphony.Tests/Commands/PlanCommandsDeriveAncestorChainTests.cs b/tests/Polyphony.Tests/Commands/PlanCommandsDeriveAncestorChainTests.cs index 3940cd17..b411cf0b 100644 --- a/tests/Polyphony.Tests/Commands/PlanCommandsDeriveAncestorChainTests.cs +++ b/tests/Polyphony.Tests/Commands/PlanCommandsDeriveAncestorChainTests.cs @@ -205,4 +205,67 @@ await SeedAsync( result.Error.ShouldNotBeNull(); result.Error.ShouldContain("Cycle detected"); } + + // ───────────────────────────────────────────────────────────────────────── + // Wire-shape regression: parent_item_id is always emitted (bug #8 — dogfood + // apex #3043, 2026-05-08). The PolyphonyJsonContext default is + // WhenWritingNull, but ParentItemId is per-property pinned to Never so + // workflow Jinja under strict_undefined can reference + // `output.parent_item_id` unconditionally without raising on a missing + // attribute. The wire shape is uniform across root-plan and descendant + // invocations. + // ───────────────────────────────────────────────────────────────────────── + + [Fact] + public async Task RootPlan_WireShape_ParentItemIdAlwaysPresentAsNull() + { + var cmd = CreateCommand(); + var (_, output) = await CaptureConsoleAsync(() => cmd.DeriveAncestorChain(100, 100)); + + // Field must be present (not elided by WhenWritingNull) with explicit null value. + output.ShouldContain("\"parent_item_id\":null"); + } + + [Fact] + public async Task DirectChildOfRoot_WireShape_ParentItemIdAlwaysPresentAsNull() + { + await SeedAsync( + new WorkItemBuilder().WithId(100).WithType("Epic").Build(), + new WorkItemBuilder().WithId(200).WithType("Issue").WithParentId(100).Build()); + + var cmd = CreateCommand(); + var (_, output) = await CaptureConsoleAsync(() => cmd.DeriveAncestorChain(100, 200)); + + // Direct children of root also have null parent_item_id (the parent IS root, + // which is implicit). Field must still be emitted explicitly. + output.ShouldContain("\"parent_item_id\":null"); + } + + [Fact] + public async Task DescendantPlan_WireShape_ParentItemIdHasIntegerValue() + { + await SeedAsync( + new WorkItemBuilder().WithId(100).WithType("Epic").Build(), + new WorkItemBuilder().WithId(200).WithType("Issue").WithParentId(100).Build(), + new WorkItemBuilder().WithId(250).WithType("Task").WithParentId(200).Build()); + + var cmd = CreateCommand(); + var (_, output) = await CaptureConsoleAsync(() => cmd.DeriveAncestorChain(100, 250)); + + // Descendant: parent_item_id must be the integer parent id (here, 200). + output.ShouldContain("\"parent_item_id\":200"); + } + + [Fact] + public async Task ErrorPath_WireShape_ParentItemIdAlwaysPresent() + { + // Even on the error path, the schema must include parent_item_id so + // workflow consumers don't have to branch the access pattern by success + // vs. failure envelope. + var cmd = CreateCommand(); + var (_, output) = await CaptureConsoleAsync(() => cmd.DeriveAncestorChain(0, 0)); + + output.ShouldContain("\"parent_item_id\":null"); + output.ShouldContain("\"error\":"); + } }