ADR-335: Preserve unknown frontmatter keys in PipelineContext round-trip

[jodavis-claude]

Work item

ADR-335: Fix PipelineContext.save()/load() in dev_team.py so a context file's full YAML frontmatter block round-trips as a dict — preserving any key PipelineContext doesn't itself declare as a typed field — instead of silently dropping every such key (working_branch, base_branch, parent_work_item, and every new field later tasks in the Concurrent Development spec will add) on every dev_team.py invocation.

Changes

• plugins/dev-team/skills/workflow-orchestrate/scripts/dev_team.py — added a module-level KNOWN_FRONTMATTER_KEYS constant listing every frontmatter key PipelineContext declares as a typed field, and a new extra_frontmatter: dict dataclass field (default {}). load() now populates extra_frontmatter from every parsed frontmatter key not in KNOWN_FRONTMATTER_KEYS. save() now writes each extra_frontmatter entry back after the typed-field lines — as a scalar key: value line, or as a key: line followed by - item bullets for list-valued entries — reusing the existing _parse_frontmatter()/_parse_sections() helpers unchanged.
• plugins/dev-team/skills/workflow-orchestrate/scripts/test_dev_team.py — added TestExtraFrontmatterRoundTrip with a make_sut() factory, a parametrized test_unknown_field_survives_load_save_roundtrip (scalar working_branch case and list-valued extra_list case), and test_prepopulated_extra_frontmatter_field_survives_save_load_roundtrip (constructs a PipelineContext with a pre-populated extra_frontmatter={"base_branch": "main"}, simulating a value set before a workflow-orchestrate run, and asserts it survives a save()/load() cycle).

Design decisions

• List-valued unknown frontmatter keys are handled, not just scalars — resolving the task brief's "known ambiguity" about list support in favor of the more future-proof, low-cost option.
• Extra/unknown keys are written back appended after all typed fields rather than interleaved at their original position — the simpler option the brief's suggested implementation shape pointed at; nothing downstream depends on frontmatter key order.
• Implemented via a genuine TDD red-green-refactor loop (tdd-tester/tdd-implementer/tdd-refactorer trio). The refactor turn extracted the implementer's first-pass inline known_keys set into the module-level KNOWN_FRONTMATTER_KEYS constant to remove drift risk against the field list save() writes, and consolidated the scalar/list-valued round-trip tests into one parametrized test.
• No E2E scenarios were added: per the missing-test-harness skill, no E2E harness exists anywhere in this repo for Python code such as dev_team.py, and the task brief's exit criteria call only for unit tests.

Testing completed

• python3 -m pytest test_dev_team.py -q from plugins/dev-team/skills/workflow-orchestrate/scripts/ — all 111 tests pass (109 pre-existing + 2 new test methods, one parametrized), confirming every previously-passing known-field round-trip is unaffected.

[github-actions]

build-and-test: Python test results

Status: ✅ Passed

Test log

[jodavis-claude]

Review summary — ADR-335

Reviewed against the task brief's exit criteria, CONTRIBUTING.md, and _doc_Projects.md.

Exit criteria — all met:

• PipelineContext.save()/load() now read/write the full frontmatter block generically: KNOWN_FRONTMATTER_KEYS names every typed field, and extra_frontmatter (populated in load(), merged back in save()) preserves everything else — including list-valued unknown keys via the existing _parse_frontmatter() multi-line - item support.
• working_branch/base_branch/parent_work_item now survive a dev_team.py round-trip, verified generically (not special-cased) by TestExtraFrontmatterRoundTrip.
• Unit tests cover both the "unknown key written directly via Edit" case (scalar + list) and the "pre-populated extra_frontmatter before a run" case (base_branch).
• Full regression: all 111 tests in test_dev_team.py pass locally, and scripts/validate.sh (build + tests) passes clean. CI (build-and-test, gate) is green on the PR.

Correctness/fault tolerance: No swallowed exceptions introduced; the existing try/except (KeyError, ValueError) around started/last_updated parsing is untouched. The extra_frontmatter merge is a straightforward dict comprehension with no failure modes on the inputs _parse_frontmatter can actually produce (str or list[str]).

Security / performance: No injection risk (local file I/O only, no shell/SQL boundary touched), no N+1 or synchronous-I/O concerns — this is a small, local, single-pass dict merge.

Documentation: No _doc_*.md describes PipelineContext's internals at this level of detail, and this is a bug fix restoring already-intended behavior (per the owning spec's Component Breakdown), not a design change — no doc update needed. Scope matches the brief exactly: only dev_team.py and test_dev_team.py touched, confirmed no other file references PipelineContext.

Minor, non-blocking observations (not requesting changes):

• extra_frontmatter: dict = field(default_factory=dict) could carry the more precise type hint dict[str, str | list[str]] the brief suggested, for a little extra self-documentation.
• Edge case not covered by a test: if extra_frontmatter is ever populated with an empty list value (not reachable from any current code path — _parse_frontmatter never produces an empty list, and no caller constructs one), save() would write a bare key: line with no - item bullets, and a subsequent load() would read that back as an empty string rather than an empty list, silently changing its type. Worth a one-line guard or a comment if this ever becomes reachable; not a real bug today since nothing produces this input.
• KNOWN_FRONTMATTER_KEYS is a mutable set(); a frozenset would better signal it's a fixed constant. Purely stylistic.

No Priority 1–4 issues found. Approving (posted as COMMENT rather than APPROVE since the PR author and reviewer share this bot account).

[jodavis-claude]

Sign-off review -- ADR-335

Checked both of jodavis's prior threads against the three follow-up commits (a4d7d7c, 64b6323, eda325f) and scanned the modified file set (pipeline_context.py (new), dev_team.py, test_pipeline_context.py (new)).

Thread: "hand-maintained KNOWN_FRONTMATTER_KEYS drift" -- resolved. eda325f derives KNOWN_FRONTMATTER_KEYS from dataclasses.fields(PipelineContext) filtered on a metadata={"frontmatter": True} tag on each frontmatter-backed field, instead of a hand-maintained set literal. Verified the derived set matches the old hand-written one and all 135 tests stay green. Resolving this thread.

Thread: "extract PipelineContext + rebuild save()/load() via TDD" -- real work, but not fully behavior-preserving; leaving open. PipelineContext is genuinely extracted into pipeline_context.py, dev_team.py now imports it instead of defining it inline (with _parse_frontmatter/_parse_sections correctly left in dev_team.py since they're still used there for unrelated PR URL section parsing), and 23 new tests in test_pipeline_context.py give real, thorough coverage of frontmatter fields, extra_frontmatter (scalar + list), every body section including the new project_configuration (first-section ordering), the write-only Logs section, and started/last_updated parse-failure fallback. 64b6323 also fixed a genuine empty-scalar-vs-list frontmatter parsing bug the extraction surfaced, with its own red test. Full regression: 135/135 tests pass locally, CI (build-and-test, gate) is green.

However, comparing the rebuilt save() line-by-line against the pre-rebuild implementation, two behaviors present in the original hand-written code did not make it into the TDD rebuild (see inline comments): save() no longer creates its target directory if missing, and no longer bumps last_updated on every save. Both were previously untested, which is presumably how they were dropped without a failing test to catch it -- but "rebuild via TDD so it's thoroughly covered" should mean at least parity with the prior implementation's behavior, not just the behaviors explicitly named in the task brief. Also noted a minor, non-blocking cosmetic loss (dropped markdown heading on fresh-file save).

Given the two real regressions, requesting changes rather than signing off yet.

[jodavis-claude]

Sign-off review — ADR-335 (second pass)

Re-reviewed PR #63 after the three commits addressing the two blocking regressions and one
cosmetic issue flagged on the prior pass.

Prior threads — all now resolved:

• save() no longer creating the parent directory — fixed in 884b6fc. path.parent.mkdir(parents=True, exist_ok=True) restored immediately before write_text(), matching pre-rebuild behavior exactly. Regression test test_save_to_path_with_missing_parent_directory_creates_it_and_writes_file genuinely exercises a missing-parent path and was confirmed failing (FileNotFoundError) before the fix per the commit message.
• save() no longer bumping last_updated — fixed in 91bcdf4. self.last_updated = datetime.datetime.now() restored at the top of save(). Regression test test_save_updates_last_updated_to_current_time_on_each_call sleeps briefly then asserts the value actually advanced — a real assertion, not a tautology.
• Dropped markdown heading (cosmetic) — fixed in e2be40d. # {work_item_id} Dev Team Context restored after the frontmatter block, matching context_template.md's format. test_save_writes_work_item_id_heading_after_frontmatter pins it down; confirmed still inert w.r.t. load()/_parse_sections().
• Overarching "extract + TDD rebuild" thread — with both regressions and the cosmetic gap closed, PipelineContext is now genuinely extracted into its own module (pipeline_context.py) with full TDD coverage and no known behavior gap versus the pre-rebuild implementation.

Scope of this pass: only pipeline_context.py and test_pipeline_context.py were touched by the three new commits (confirmed via git diff 884b6fc~1..HEAD --stat). No other file changed, so no other file needed re-scanning.

New issues found: none. Each fix is minimal, restores the exact pre-rebuild behavior, and ships with a dedicated regression test written TDD-first (per the commit messages, each test was confirmed failing before its fix). Test structure follows this repo's conventions (AAA, make_sut(), descriptive parametrize ids). Full suite passes locally: 138/138 (27 test_pipeline_context.py + 111 test_dev_team.py). CI (build-and-test, gate) is green on the PR.

Sign-off: approved.

[jodavis-claude]

Both review threads have been addressed and resolved:

• KNOWN_FRONTMATTER_KEYS is now derived from PipelineContext dataclass field metadata instead of hand-maintained (commit eda325f).
• PipelineContext has been extracted into its own module (pipeline_context.py) and rebuilt via a genuine TDD red-green-refactor loop (commit a4d7d7c), wired into dev_team.py (commit 64b6323). A sign-off review caught two real regressions the rebuild introduced (missing mkdir on save, last_updated not bumping) plus a cosmetic dropped heading — all three fixed with regression tests (884b6fc, 91bcdf4, e2be40d).

138/138 tests pass, CI is green. Re-requesting review — ready for another look whenever you have time.