From bc120a7a9c25c8d0c7949d887fafba09e25788b8 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 2 Oct 2026 16:00:03 +0200 Subject: [PATCH 01/13] fix(codex): preserve native agent model settings Preserve supplied model and model_reasoning_effort strings in generated agent TOML. Diagnose unsupported metadata and non-string settings without changing other targets. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../instructions-and-agents.md | 21 ++- .../skills/apm-usage/package-authoring.md | 28 +++ src/apm_cli/integration/agent_integrator.py | 42 ++++- .../test_codex_agent_tool_scope_contract.py | 41 ++++- .../unit/integration/test_agent_integrator.py | 166 ++++++++++++++++++ 5 files changed, 291 insertions(+), 7 deletions(-) diff --git a/docs/src/content/docs/producer/author-primitives/instructions-and-agents.md b/docs/src/content/docs/producer/author-primitives/instructions-and-agents.md index 3da3fcc114..5d00f98707 100644 --- a/docs/src/content/docs/producer/author-primitives/instructions-and-agents.md +++ b/docs/src/content/docs/producer/author-primitives/instructions-and-agents.md @@ -168,6 +168,7 @@ for... | `name` | recommended | Display name; defaults to filename stem | | `description` | yes | Used by Cascade and Copilot to decide when to surface the agent | | `model` | optional | Pinned model the harness should switch to when invoked | +| `model_reasoning_effort` | optional | Native Codex reasoning effort, supplied as a string | | `tools` | optional | Whitelist of tools the persona may call | | `color` | optional | Display color for harnesses that render it (Copilot, Claude, OpenCode). OpenCode requires a `#rgb`/`#rrggbb` hex literal or one of its theme names; see "Common pitfalls" below | | `handoffs` | optional | List of agent names (or VS Code structured handoff objects) this agent can hand off to | @@ -182,9 +183,21 @@ of the supported capability tags (`read`, `write`, `shell`, `web`, `*`). If any unsupported tag is present, the agent is not deployed at all -- APM fails closed. Kiro may warn and fall back if the specified `model` is unavailable; APM passes the value through without -validation. Codex translates only `name`, `description`, and the -Markdown body; APM does not yet generate the complete per-agent MCP -transport definitions needed to preserve `model` or `tools`. When +validation. + +Codex receives `name`, `description`, and the Markdown body as +`developer_instructions`. Supplied string values for `model` and +`model_reasoning_effort` become top-level keys in the generated agent TOML, +as documented in the [Codex custom agent format](https://developers.openai.com/codex/subagents/#custom-agents). +Omitted fields remain absent; APM does not translate model names, choose +defaults, or validate which models and effort values your Codex supports. +Non-string values are dropped with a warning naming the field. + +Other frontmatter fields are dropped with a warning that APM does not +translate them for Codex. This includes other native Codex settings and +any `codex:` block; there is no namespaced or arbitrary passthrough. +Remove unnecessary fields, or do not rely on their settings in the +generated agent. APM still cannot preserve `tools` restrictions. When `tools` is present, `apm install` warns that the generated agent may inherit every project or session MCP server. Remove `tools` if unrestricted access is intentional; otherwise, do not use the @@ -222,7 +235,7 @@ offending package and field so you can fix the source. | grok-build | `.grok/agents/.md` | verbatim | | cursor | `.cursor/agents/.md` | verbatim | | opencode | `.opencode/agents/.md` | verbatim | -| codex | `.codex/agents/.toml` | `name` and `description` -> TOML; body becomes `developer_instructions`; unsupported `tools` emits a warning | +| codex | `.codex/agents/.toml` | `name`, `description`, and supplied string `model` / `model_reasoning_effort` -> top-level TOML; body becomes `developer_instructions`; dropped metadata emits warnings | | kiro | `.kiro/agents/.md` | `description`, `model`, `tools` kept; `name` and unknown fields stripped; identity from path; fail closed on unsupported tools (ref: [kiro.dev/docs/custom-agents](https://kiro.dev/docs/custom-agents/), accessed 2026-08-03) | | grok-build | `.grok/agents/.md` | verbatim | | windsurf | not deployed | Windsurf has no agents primitive -- author personas as skills (Cascade auto-invokes by description) | diff --git a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md index ba6026039f..c68260a80d 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -474,6 +474,34 @@ instructions: | --- ``` +#### Codex target: native model settings + +Codex agents deploy to `.codex/agents/.toml`. APM writes `name`, +`description`, and the Markdown body as `developer_instructions`. +String `model` and `model_reasoning_effort` frontmatter values are +preserved as top-level TOML keys: + +```markdown +--- +name: reviewer +description: Reviews code +model: gpt-5.6-sol +model_reasoning_effort: high +--- +Review the requested change. +``` + +Absent settings stay absent. APM does not translate model names, choose +defaults, or validate model availability or supported effort values. +Non-string values are dropped with a diagnostic. Other frontmatter, +including other native Codex settings and a `codex:` block, is dropped +with a diagnostic; APM provides no arbitrary or namespaced passthrough. +Remove unnecessary fields or do not rely on their settings in the +generated agent. `tools` restrictions still cannot be preserved: the +agent may inherit all project/session MCP servers. Remove `tools` only +if unrestricted access is intentional; otherwise do not use the generated +agent with Codex. + #### OpenCode target: frontmatter constraints OpenCode (`target: opencode`, deploys to `.opencode/agents/`) parses diff --git a/src/apm_cli/integration/agent_integrator.py b/src/apm_cli/integration/agent_integrator.py index d6d38f027f..21eac49720 100644 --- a/src/apm_cli/integration/agent_integrator.py +++ b/src/apm_cli/integration/agent_integrator.py @@ -463,8 +463,8 @@ def _write_codex_agent( ) -> None: """Transform an ``.agent.md`` file to Codex ``.toml`` format. - Parses YAML frontmatter for ``name`` and ``description``, uses - the markdown body as ``developer_instructions``. + Preserves ``name``, ``description``, and native model settings; + uses the markdown body as ``developer_instructions``. """ if source.is_symlink(): raise ValueError(f"Refusing to read symlink source: {source}") @@ -477,6 +477,8 @@ def _write_codex_agent( name = name[: -len(".agent")] description = "" body = content + model_fields = ("model", "model_reasoning_effort") + model_settings: dict[str, str] = {} fm_match = AgentIntegrator._FRONTMATTER_RE.match(content) if fm_match: @@ -486,6 +488,41 @@ def _write_codex_agent( if isinstance(fm, dict): name = fm.get("name", name) description = fm.get("description", description) + for field in model_fields: + if field not in fm: + continue + if isinstance(fm[field], str): + model_settings[field] = fm[field] + elif diagnostics is not None: + diagnostics.lossy_agent_compilation( + message=( + f"Codex agent {printable_ascii_text(source.name)}: frontmatter " + f"field '{field}' must be a string and was dropped." + ), + package=printable_ascii_text(package_name), + detail=( + f"Fix: set '{field}' to a string in the source agent, " + "then rerun 'apm install'." + ), + ) + dropped_fields = [ + f"'{printable_ascii_text(str(field))}'" + for field in fm + if field not in {"name", "description", "tools", *model_fields} + ] + if dropped_fields and diagnostics is not None: + diagnostics.lossy_agent_compilation( + message=( + f"Codex agent {printable_ascii_text(source.name)}: frontmatter " + f"fields {', '.join(dropped_fields)} were dropped; " + "this metadata is not translated by APM for Codex." + ), + package=printable_ascii_text(package_name), + detail=( + "Fix: remove these fields if unnecessary; otherwise do not rely on " + "their settings in the generated Codex agent." + ), + ) else: AgentIntegrator._warn_codex_unverified_scope( diagnostics, @@ -513,6 +550,7 @@ def _write_codex_agent( doc = { "name": name, "description": description, + **model_settings, "developer_instructions": body.strip(), } write_text_lf(target, _toml.dumps(doc)) diff --git a/tests/integration/test_codex_agent_tool_scope_contract.py b/tests/integration/test_codex_agent_tool_scope_contract.py index bb908619fe..36dd293119 100644 --- a/tests/integration/test_codex_agent_tool_scope_contract.py +++ b/tests/integration/test_codex_agent_tool_scope_contract.py @@ -8,7 +8,7 @@ import pytest import tomllib -from apm_cli.utils.yaml_io import dump_yaml, load_yaml +from apm_cli.utils.yaml_io import dump_yaml, load_yaml, yaml_to_str from tests.utils.apm_lifecycle_runner import ApmLifecycleRunner, CommandResult from tests.utils.isolated_apm_environment import IsolatedApmEnvironment from tests.utils.local_package import LocalPackageFactory @@ -42,6 +42,7 @@ def _install_codex_agent( *, name: str, tools: list[str] | None, + metadata: dict[str, str] | None = None, ) -> tuple[CommandResult, Path]: """Author and install one Codex project through the packaged CLI boundary.""" isolated = IsolatedApmEnvironment.create(root, base_env=dict(os.environ)) @@ -58,6 +59,8 @@ def _install_codex_agent( ] if tools is not None: frontmatter.append("tools: [read, search, 'allowed-demo/*']") + if metadata: + frontmatter.append(yaml_to_str(metadata).rstrip("\n")) frontmatter.extend(("---", "", "Review the requested change.", "")) factory.add_agent(project, name, "\n".join(frontmatter)) @@ -116,6 +119,42 @@ def test_codex_agent_tool_scope_is_never_silently_lost( assert unscoped_agent["name"] == "plain-reviewer" assert "tools" not in unscoped_agent assert "mcp_servers" not in unscoped_agent + assert "model" not in unscoped_agent + assert "model_reasoning_effort" not in unscoped_agent assert "[!]" not in unscoped_output assert "lossy agent compilation" not in unscoped_output assert "frontmatter field 'tools' was dropped" not in " ".join(unscoped_output.split()) + + +def test_codex_agent_native_models_and_dropped_metadata_reach_cli_output( + tmp_path: Path, + apm_binary_path: Path, +) -> None: + """Install preserves model settings and reports APM's remaining translation loss.""" + result, project = _install_codex_agent( + tmp_path / "native-model", + apm_binary_path, + name="model-reviewer", + tools=None, + metadata={ + "model": "gpt-5.6-sol", + "model_reasoning_effort": "high", + "model_verbosity": "low", + }, + ) + assert result.returncode == 0, result.stdout + result.stderr + assert _read_toml(project / ".codex" / "agents" / "model-reviewer.toml") == { + "name": "model-reviewer", + "description": "Codex agent model-reviewer", + "developer_instructions": "Review the requested change.", + "model": "gpt-5.6-sol", + "model_reasoning_effort": "high", + } + output = " ".join((result.stdout + result.stderr).split()) + assert "[!]" in output + assert "1 lossy agent compilation warning" in output + assert "model-reviewer.agent.md" in output + assert "fields 'model_verbosity' were dropped" in output + assert "not translated by APM for Codex" in output + assert "otherwise do not rely on" in output + assert "field 'tools' was dropped" not in output diff --git a/tests/unit/integration/test_agent_integrator.py b/tests/unit/integration/test_agent_integrator.py index 4cc98ced7a..885f85b8a4 100644 --- a/tests/unit/integration/test_agent_integrator.py +++ b/tests/unit/integration/test_agent_integrator.py @@ -5,6 +5,9 @@ from pathlib import Path from unittest.mock import Mock +import pytest +import tomllib + from apm_cli.integration import AgentIntegrator from apm_cli.models.apm_package import APMPackage, GitReferenceType, PackageInfo, ResolvedReference from apm_cli.utils.diagnostics import ( @@ -12,6 +15,7 @@ CATEGORY_WARNING, DiagnosticCollector, ) +from apm_cli.utils.yaml_io import yaml_to_str class TestAgentIntegrator: @@ -1167,6 +1171,168 @@ def test_codex_agent_target_filename_is_toml(self): filename = integrator.get_target_filename_for_target(source, "pkg", codex) assert filename == "test.toml" + @pytest.mark.parametrize( + "settings", + [ + {}, + {"model": "gpt-5.6-sol"}, + {"model_reasoning_effort": "high"}, + {"model": "gpt-5.6-sol", "model_reasoning_effort": "high"}, + {"model": 'custom"model\\variant', "model_reasoning_effort": "future-effort"}, + ], + ) + def test_codex_native_settings_reach_generated_agent(self, settings: dict[str, str]) -> None: + """Preserve supplied strings at TOML top level without inventing defaults.""" + from apm_cli.integration.targets import KNOWN_TARGETS + + package_dir = self.root / "package" + agents_dir = package_dir / ".apm" / "agents" + agents_dir.mkdir(parents=True) + frontmatter = {"name": "reviewer", "description": "Review code", **settings} + (agents_dir / "reviewer.agent.md").write_text( + f"---\n{yaml_to_str(frontmatter)}---\nReview changes.\n", encoding="utf-8" + ) + diagnostics = DiagnosticCollector() + + result = AgentIntegrator().integrate_agents_for_target( + KNOWN_TARGETS["codex"], + self._create_package_info(package_dir), + self.root, + diagnostics=diagnostics, + ) + + target = self.root / ".codex" / "agents" / "reviewer.toml" + assert result.target_paths == [target] + assert tomllib.loads(target.read_text(encoding="utf-8")) == { + "name": "reviewer", + "description": "Review code", + "developer_instructions": "Review changes.", + **settings, + } + assert diagnostics.by_category() == {} + + @pytest.mark.parametrize("field", ["model", "model_reasoning_effort"]) + @pytest.mark.parametrize("value", [None, False, 7, ["high"], {"value": "high"}]) + def test_codex_non_string_settings_are_diagnosed(self, field: str, value: object) -> None: + """Do not stringify invalid YAML shapes or silently drop null settings.""" + source = self.root / "invalid-setting.agent.md" + source.write_text( + f"---\n{yaml_to_str({'name': 'reviewer', field: value})}---\nReview changes.\n", + encoding="utf-8", + ) + target = self.root / "reviewer.toml" + diagnostics = DiagnosticCollector() + + AgentIntegrator._write_codex_agent( + source, target, diagnostics=diagnostics, package_name="test-pkg" + ) + + assert tomllib.loads(target.read_text(encoding="utf-8")) == { + "name": "reviewer", + "description": "", + "developer_instructions": "Review changes.", + } + warnings = diagnostics.by_category().get(CATEGORY_AGENT_LOSSY_COMPILATION, []) + assert len(warnings) == 1 + assert field in warnings[0].message + assert "must be a string" in warnings[0].message + assert "was dropped" in warnings[0].message + assert "source agent" in warnings[0].detail + assert "apm install" in warnings[0].detail + + def test_codex_unsupported_metadata_is_diagnosed_without_passthrough( + self, capsys: pytest.CaptureFixture[str] + ) -> None: + """Name dropped fields without claiming Codex lacks their native semantics.""" + source = self.root / "metadata.agent.md" + unsupported = { + "model_reasoning_summary": "detailed", + "model_verbosity": "low", + "personality": "friendly", + "service_tier": "fast", + "codex": {"model": "do-not-passthrough"}, + "developer_instructions": "Do not replace the Markdown body.", + "color": "cyan", + "bad\x1b[31m\nkey": "do-not-print-values", + 7: "non-string-key", + } + frontmatter = { + "name": "reviewer", + "model": "gpt-5.6-sol", + "model_reasoning_effort": "high", + "tools": [], + **unsupported, + } + source.write_text( + f"---\n{yaml_to_str(frontmatter)}---\nReview changes.\n", encoding="utf-8" + ) + target = self.root / "reviewer.toml" + diagnostics = DiagnosticCollector() + + AgentIntegrator._write_codex_agent( + source, target, diagnostics=diagnostics, package_name="evil\x1b[31mpkg\nnext" + ) + + assert tomllib.loads(target.read_text(encoding="utf-8")) == { + "name": "reviewer", + "description": "", + "developer_instructions": "Review changes.", + "model": "gpt-5.6-sol", + "model_reasoning_effort": "high", + } + warnings = diagnostics.by_category().get(CATEGORY_AGENT_LOSSY_COMPILATION, []) + assert len(warnings) == 2 + metadata_warning = next( + warning for warning in warnings if "not translated by APM" in warning.message + ) + for field in list(unsupported)[:7]: + assert f"'{field}'" in metadata_warning.message + assert "were dropped" in metadata_warning.message + assert "otherwise do not rely on" in metadata_warning.detail + tools_warning = next(warning for warning in warnings if "field 'tools'" in warning.message) + assert "project/session MCP servers" in tools_warning.message + + diagnostics.render_summary() + output = capsys.readouterr().out + assert "[!]" in output + assert "metadata.agent.md" in output + assert "do-not-print-values" not in output + assert "\x1b" not in output + assert "pkg\nnext" not in output + assert all(character in "\n\r\t" or 32 <= ord(character) <= 126 for character in output) + + @pytest.mark.parametrize( + "target_name", ["copilot", "claude", "cursor", "grok-build", "opencode"] + ) + def test_codex_metadata_change_preserves_other_targets(self, target_name: str) -> None: + """Codex filtering must not leak into verbatim target deployment.""" + from apm_cli.integration.targets import KNOWN_TARGETS + + package_dir = self.root / "package" + agents_dir = package_dir / ".apm" / "agents" + agents_dir.mkdir(parents=True) + content = ( + "---\nname: reviewer\nmodel: native-model\nmodel_reasoning_effort: high\n" + "model_verbosity: low\n---\nReview changes.\n" + ) + source = agents_dir / "reviewer.agent.md" + source.write_text(content, encoding="utf-8") + profile = KNOWN_TARGETS[target_name] + (self.root / profile.root_dir).mkdir(exist_ok=True) + diagnostics = DiagnosticCollector() + + result = AgentIntegrator().integrate_agents_for_target( + profile, + self._create_package_info(package_dir), + self.root, + diagnostics=diagnostics, + ) + + assert result.files_integrated == 1 + assert result.target_paths[0].read_text(encoding="utf-8") == content + assert source.read_text(encoding="utf-8") == content + assert diagnostics.by_category() == {} + def test_codex_agent_tools_scope_emits_lossy_compilation_warning(self, capsys): """Codex installs must not silently discard agent tool restrictions.""" from apm_cli.integration.targets import KNOWN_TARGETS From 1266717d40829e1e081f6568bb7c0132c0b513fc Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 2 Oct 2026 16:08:48 +0200 Subject: [PATCH 02/13] docs: record Codex model preservation fix Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e49c83b56..6f0330d3bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Codex agent conversion preserves native `model` and `model_reasoning_effort` settings and warns about dropped metadata instead of silently losing it. (#3150) - `apm install` reports the original resolution error for a dependency whose git ref cannot be resolved, instead of failing later while annotating the update plan. (#3142) - Repeat `apm install` runs keep existing MCP servers from custom (`registry:`) registries instead of reconfiguring them, preserving their authored headers. (#3142) From e0b25820f065d81a67c51b0c43733803729ec10a Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 2 Oct 2026 18:28:50 +0200 Subject: [PATCH 03/13] fix(codex): bound dropped-fields diagnostic and derive leakage-guard targets from catalog - Cap the dropped-fields diagnostic to 5 named keys with an '(and N more)' summary so hostile/oversized frontmatter cannot blow up a single CLI diagnostic line (folds copilot-pull-request-reviewer finding 4166618929). - Derive test_codex_metadata_change_preserves_other_targets' parametrization from KNOWN_TARGETS instead of a hardcoded literal list, so a future verbatim-copy target is automatically covered by the Codex-leakage regression test (excludes codex_agent/kiro_agent and agent-less targets, which are not byte-identical copies). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/integration/agent_integrator.py | 13 ++++++- .../unit/integration/test_agent_integrator.py | 34 ++++++++++++++++--- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/apm_cli/integration/agent_integrator.py b/src/apm_cli/integration/agent_integrator.py index 21eac49720..6379aedecd 100644 --- a/src/apm_cli/integration/agent_integrator.py +++ b/src/apm_cli/integration/agent_integrator.py @@ -412,6 +412,12 @@ def _warn_opencode_frontmatter( re.DOTALL, ) + # Bound the dropped-fields diagnostic to a fixed number of named keys so + # hostile/oversized frontmatter (many keys or very long key names) cannot + # blow up a single CLI diagnostic line; the remainder is summarized by + # count instead of enumerated. + _MAX_DROPPED_FIELDS_SHOWN = 5 + @staticmethod def _warn_codex_unverified_scope( diagnostics: DiagnosticCollector | None, @@ -511,10 +517,15 @@ def _write_codex_agent( if field not in {"name", "description", "tools", *model_fields} ] if dropped_fields and diagnostics is not None: + shown_fields = dropped_fields[: AgentIntegrator._MAX_DROPPED_FIELDS_SHOWN] + remaining = len(dropped_fields) - len(shown_fields) + fields_text = ", ".join(shown_fields) + if remaining > 0: + fields_text += f" (and {remaining} more)" diagnostics.lossy_agent_compilation( message=( f"Codex agent {printable_ascii_text(source.name)}: frontmatter " - f"fields {', '.join(dropped_fields)} were dropped; " + f"fields {fields_text} were dropped; " "this metadata is not translated by APM for Codex." ), package=printable_ascii_text(package_name), diff --git a/tests/unit/integration/test_agent_integrator.py b/tests/unit/integration/test_agent_integrator.py index 885f85b8a4..007ebde016 100644 --- a/tests/unit/integration/test_agent_integrator.py +++ b/tests/unit/integration/test_agent_integrator.py @@ -1097,6 +1097,26 @@ def test_sync_integration_opencode_handles_missing_dir(self): assert result["errors"] == 0 +def _verbatim_copy_agent_targets() -> list[str]: + """Non-codex, non-kiro targets whose agent deploy is a verbatim copy. + + Derived from ``KNOWN_TARGETS`` (instead of a hardcoded literal list) so a + future verbatim-copy target is automatically covered by the Codex-leakage + regression test below. ``codex_agent`` is this suite's own transform; + ``kiro_agent`` renders/filters the markdown body and is not byte-identical + to the source, so both are excluded on purpose, as are targets with no + ``agents`` primitive mapping at all (they never reach the copy path). + """ + from apm_cli.integration.targets import KNOWN_TARGETS + + return sorted( + name + for name, profile in KNOWN_TARGETS.items() + if (mapping := profile.primitives.get("agents")) is not None + and mapping.format_id not in {"codex_agent", "kiro_agent"} + ) + + class TestCodexAgentIntegration: """Tests for Codex TOML agent transformation.""" @@ -1285,8 +1305,16 @@ def test_codex_unsupported_metadata_is_diagnosed_without_passthrough( metadata_warning = next( warning for warning in warnings if "not translated by APM" in warning.message ) - for field in list(unsupported)[:7]: + # req: the dropped-fields list is bounded (AgentIntegrator._MAX_DROPPED_FIELDS_SHOWN) + # so hostile/oversized frontmatter cannot blow up a single diagnostic line. 9 + # unsupported keys are supplied; only the first 5 are named, the rest are summarized. + shown, elided = list(unsupported)[:5], list(unsupported)[5:] + assert len(elided) == 4 + for field in shown: assert f"'{field}'" in metadata_warning.message + for field in elided: + assert f"'{field}'" not in metadata_warning.message + assert f"(and {len(elided)} more)" in metadata_warning.message assert "were dropped" in metadata_warning.message assert "otherwise do not rely on" in metadata_warning.detail tools_warning = next(warning for warning in warnings if "field 'tools'" in warning.message) @@ -1301,9 +1329,7 @@ def test_codex_unsupported_metadata_is_diagnosed_without_passthrough( assert "pkg\nnext" not in output assert all(character in "\n\r\t" or 32 <= ord(character) <= 126 for character in output) - @pytest.mark.parametrize( - "target_name", ["copilot", "claude", "cursor", "grok-build", "opencode"] - ) + @pytest.mark.parametrize("target_name", _verbatim_copy_agent_targets()) def test_codex_metadata_change_preserves_other_targets(self, target_name: str) -> None: """Codex filtering must not leak into verbatim target deployment.""" from apm_cli.integration.targets import KNOWN_TARGETS From a0fcdfba779bed5e0674630e6d452d8c07fe34db Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 2 Oct 2026 18:30:19 +0200 Subject: [PATCH 04/13] chore: resolve CHANGELOG.md merge with origin/main (v0.33.0 release) Keep main's released [0.33.0] section verbatim and move this PR's own entry (Codex native model settings preservation, #3150) to [Unreleased] since it has not shipped in 0.33.0. Drops the duplicate pre-release entries that a 3-way merge inserted into the already-released section. --- CHANGELOG.md | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c724961a91..946fe4538a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Codex agent conversion preserves native `model` and `model_reasoning_effort` settings and warns about dropped metadata instead of silently losing it. (#3150) + ## [0.33.0] - 2026-10-02 ### Added @@ -15,20 +19,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Codex agent conversion preserves native `model` and `model_reasoning_effort` settings and warns about dropped metadata instead of silently losing it. (#3150) -- `apm install` reports the original resolution error for a dependency whose git ref cannot be resolved, instead of failing later while annotating the update plan. (#3142) -- Repeat `apm install` runs keep existing MCP servers from custom (`registry:`) registries instead of reconfiguring them, preserving their authored headers. (#3142) - -- `apm audit` discovers tracked and untracked target-native prompts without treating executable commands as prompt text; unreadable or unsupported recognized formats now fail with incomplete coverage, and shared/user configuration cannot be auto-stripped. Review named incomplete or protected locations manually before re-auditing. Proposed `specs/openapm-v0.1.md` audit contract remains subject to specification adoption. -- by @lkshrk (#2962) -- Cursor MCP configuration now preserves native runtime environment references and authored static values, normalizes scalar environment values, and omits null entries. (by @icecold009, #3070) -- Global skill installs work through `HOME`/`APM_HOME` directory aliases without relaxing package or destination safety; frozen installs reject ref, pin, provider and transport drift, and unseeded mutable refs resolve upstream. After an intentional declaration change, review it and run `apm install --update`. (by @DaveMeadAdjust, #2876) -- GitHub MCP authentication now preserves explicit `Authorization` headers and writes the selected environment-variable reference on runtime-capable targets instead of resolving the credential into generated config. (#3103) -- OpenCode MCP installs and reinstalls preserve an explicitly supplied `enabled` value and JSON type; omitted values still default to `true` - by @dajiaohuang (#3102). -- `apm compile --target claude` imports dependency-root `CLAUDE.md` files at any metadata-backed materialization depth, including ADO and nested GitLab paths, and resolves transitive imports with redirected `--root` output. Frozen replay also reads the selected installation's lockfile. (by @vyrnsynx, #2952) -- Codex MCP headers now use native runtime environment references instead of literal placeholders; unsupported references are skipped with a warning. Unchanged reinstalls preserve existing entries; switch the affected header between `${VAR}` and `${env:VAR}` and reinstall with the same scope and targets to refresh an older managed entry. -- by @edenfunf (#3042) -- Claude MCP redeclarations drop stale transport fields and repair mixed entries when rewritten, while preserving partial updates and unmanaged configuration. -- by @edenfunf (#3041) -- Cursor rules now use comma-joined `globs` and readable descriptions, while retaining safe escaping for control characters. (by @YGuyomar, #3011) -- Copilot user-scope installs restore modular `~/.copilot/instructions/**/*.instructions.md` deployment (mirroring the project-scope layout) instead of tracking a single concatenated `copilot-instructions.md`. (by @kilianpaquier, #2317) - `apm install` reports the original resolution error for a dependency whose git ref cannot be resolved, and repeat installs keep MCP servers from custom (`registry:`) registries instead of reconfiguring them. (#3142) - `apm audit` bounds its hidden-Unicode walk of deployed files to primitive directories and discovers target-native prompts without treating executable commands as prompt text; unreadable or unsupported formats now report incomplete coverage. (by @lkshrk, #2962) - Cursor MCP configuration keeps native `${env:NAME}` references and authored static values instead of resolving secrets into `.cursor/mcp.json`. (by @icecold009, #3116) From 6f33b8b5fa419bae5d1882759e11edb3d417030d Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 2 Oct 2026 19:26:54 +0200 Subject: [PATCH 05/13] fix(codex): bound individual dropped-field key display length The existing _MAX_DROPPED_FIELDS_SHOWN cap bounds the *number* of named dropped frontmatter keys but not an individual key's *length*. A single hostile/oversized key name (e.g. 20000 chars) still produced an unbounded diagnostic line. Add _MAX_DROPPED_FIELD_KEY_LEN to truncate each displayed key independently of the count cap, and stop pre-rendering every dropped key (sanitizing/formatting) before slicing to the shown subset. Also relocates the malformed/control-character key fixture in the existing diagnosed-without-passthrough test into the shown range so displayed-key sanitization is actually exercised (it was previously always in the elided tail), and adds a focused regression test for the oversized-key case with mutation-break proof. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/integration/agent_integrator.py | 44 +++++++++++++---- .../unit/integration/test_agent_integrator.py | 47 ++++++++++++++++++- 2 files changed, 81 insertions(+), 10 deletions(-) diff --git a/src/apm_cli/integration/agent_integrator.py b/src/apm_cli/integration/agent_integrator.py index 6379aedecd..2da8e36f40 100644 --- a/src/apm_cli/integration/agent_integrator.py +++ b/src/apm_cli/integration/agent_integrator.py @@ -413,10 +413,29 @@ def _warn_opencode_frontmatter( ) # Bound the dropped-fields diagnostic to a fixed number of named keys so - # hostile/oversized frontmatter (many keys or very long key names) cannot - # blow up a single CLI diagnostic line; the remainder is summarized by - # count instead of enumerated. + # hostile/oversized frontmatter (many keys) cannot blow up a single CLI + # diagnostic line; the remainder is summarized by count instead of + # enumerated. _MAX_DROPPED_FIELDS_SHOWN = 5 + # Bound each individually-named key to a fixed display length so a + # single hostile/oversized key name cannot blow up a single CLI + # diagnostic line either; combined with _MAX_DROPPED_FIELDS_SHOWN this + # bounds the whole diagnostic's total length. + _MAX_DROPPED_FIELD_KEY_LEN = 60 + + @staticmethod + def _display_dropped_field_key(field: object) -> str: + """Render one dropped frontmatter key name, bounded and ASCII-safe. + + Sanitizes control/non-ASCII characters first, then truncates the + *displayed* key (never the full value) so a single oversized key + name cannot blow up the diagnostic line. + """ + text = printable_ascii_text(str(field)) + max_len = AgentIntegrator._MAX_DROPPED_FIELD_KEY_LEN + if len(text) > max_len: + text = text[:max_len] + "...(truncated)" + return text @staticmethod def _warn_codex_unverified_scope( @@ -511,14 +530,23 @@ def _write_codex_agent( "then rerun 'apm install'." ), ) - dropped_fields = [ - f"'{printable_ascii_text(str(field))}'" + # Collect only the raw dropped keys here (no formatting) + # so hostile frontmatter with many keys does not force + # rendering every key just to discard most of them below. + dropped_field_names = [ + field for field in fm if field not in {"name", "description", "tools", *model_fields} ] - if dropped_fields and diagnostics is not None: - shown_fields = dropped_fields[: AgentIntegrator._MAX_DROPPED_FIELDS_SHOWN] - remaining = len(dropped_fields) - len(shown_fields) + if dropped_field_names and diagnostics is not None: + shown_names = dropped_field_names[ + : AgentIntegrator._MAX_DROPPED_FIELDS_SHOWN + ] + remaining = len(dropped_field_names) - len(shown_names) + shown_fields = [ + f"'{AgentIntegrator._display_dropped_field_key(field)}'" + for field in shown_names + ] fields_text = ", ".join(shown_fields) if remaining > 0: fields_text += f" (and {remaining} more)" diff --git a/tests/unit/integration/test_agent_integrator.py b/tests/unit/integration/test_agent_integrator.py index 007ebde016..831978f10c 100644 --- a/tests/unit/integration/test_agent_integrator.py +++ b/tests/unit/integration/test_agent_integrator.py @@ -1270,10 +1270,13 @@ def test_codex_unsupported_metadata_is_diagnosed_without_passthrough( "model_verbosity": "low", "personality": "friendly", "service_tier": "fast", + # req: a malformed/control-character key must land inside the + # shown range (not only the elided tail) so sanitization of a + # *displayed* key name is actually exercised by this test. + "bad\x1b[31m\nkey": "do-not-print-values", "codex": {"model": "do-not-passthrough"}, "developer_instructions": "Do not replace the Markdown body.", "color": "cyan", - "bad\x1b[31m\nkey": "do-not-print-values", 7: "non-string-key", } frontmatter = { @@ -1311,12 +1314,22 @@ def test_codex_unsupported_metadata_is_diagnosed_without_passthrough( shown, elided = list(unsupported)[:5], list(unsupported)[5:] assert len(elided) == 4 for field in shown: - assert f"'{field}'" in metadata_warning.message + if isinstance(field, str) and field.isidentifier(): + assert f"'{field}'" in metadata_warning.message + # req: the malformed/control-character key is in the shown range (see + # reordering above); its sanitized form must be named, never the raw + # control bytes. + assert "'bad?[31m?key'" in metadata_warning.message for field in elided: assert f"'{field}'" not in metadata_warning.message assert f"(and {len(elided)} more)" in metadata_warning.message assert "were dropped" in metadata_warning.message assert "otherwise do not rely on" in metadata_warning.detail + # req: an individually oversized key name is also bounded + # (AgentIntegrator._MAX_DROPPED_FIELD_KEY_LEN), independent of the + # fixed-count cap above, so a single hostile key cannot blow up the + # diagnostic line either. + assert len(metadata_warning.message) < 600 tools_warning = next(warning for warning in warnings if "field 'tools'" in warning.message) assert "project/session MCP servers" in tools_warning.message @@ -1329,6 +1342,36 @@ def test_codex_unsupported_metadata_is_diagnosed_without_passthrough( assert "pkg\nnext" not in output assert all(character in "\n\r\t" or 32 <= ord(character) <= 126 for character in output) + def test_codex_dropped_field_with_oversized_key_name_is_bounded(self) -> None: + """A single hostile/oversized key name must not blow up the diagnostic. + + The fixed-count cap (_MAX_DROPPED_FIELDS_SHOWN) alone does not bound + an individual key's *length*; a frontmatter with as few as one + unsupported key but a very long name must still produce a bounded + message. + """ + source = self.root / "oversized-key.agent.md" + oversized_key = "x" * 20_000 + frontmatter = {"name": "reviewer", oversized_key: "value"} + source.write_text( + f"---\n{yaml_to_str(frontmatter)}---\nReview changes.\n", encoding="utf-8" + ) + target = self.root / "reviewer.toml" + diagnostics = DiagnosticCollector() + + AgentIntegrator._write_codex_agent( + source, target, diagnostics=diagnostics, package_name="test-pkg" + ) + + warnings = diagnostics.by_category().get(CATEGORY_AGENT_LOSSY_COMPILATION, []) + assert len(warnings) == 1 + message = warnings[0].message + # req: the displayed key itself is truncated, not merely the number + # of named keys; the raw 20000-char key must never appear whole. + assert oversized_key not in message + assert "...(truncated)" in message + assert len(message) < 300 + @pytest.mark.parametrize("target_name", _verbatim_copy_agent_targets()) def test_codex_metadata_change_preserves_other_targets(self, target_name: str) -> None: """Codex filtering must not leak into verbatim target deployment.""" From 37717023179cc6a6e6453d187a9deba4b83ddb4e Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Sat, 3 Oct 2026 20:57:32 +0200 Subject: [PATCH 06/13] spec: add req-tg-015 for Codex native model-settings + bounded diagnostic Closes the Spec conformance gate gap on PR #3150 (issue #3126) by adding a new normative requirement, req-tg-015, documenting the already-shipped Codex-native model/model_reasoning_effort preservation and bounded dropped-metadata diagnostic behavior in _write_codex_agent. No src/** production change: the existing behavior already satisfies this clause. - Spec: new req-tg-015 anchor in Section 8.5.1 (Lossy agent conversion), two normative MUST clauses (field preservation + bounded diagnostic), editorial note. Updated Section 8.7, Section 11.3.2 consumer enumerations, Appendix C table + footer, Section 1.3 statement count (125 -> 126, 121 MUST), and Appendix D revision history (0.1.43 proposed). - Manifest: new req-tg-015 entry in docs/public/specs/manifests/openapm-v0.1.requirements.yml. - Tests: new req-tg-015 conformance test in tests/spec_conformance/test_manifest_reqs.py exercising real generated Codex TOML output (parsed via tomllib) for both/one/ neither field present, non-string dropped value, an 8-key unsupported-metadata diagnostic bounded on both count (5 shown + 3 elided) and per-key length (20000-char key truncated), no value leakage, ASCII-safety for a control-character key, unaffected req-tg-006 tools diagnostic, and unaffected non-Codex targets. - CHANGELOG.md: Changed entry noting the new spec section. - CONFORMANCE.json / CONFORMANCE.md: regenerated via tests.spec_conformance.gen_statement. Authorized by spec-conformance-20261003T182847Z-pr3150, addendum https://github.com/microsoft/apm/issues/3126#issuecomment-5972216780, coordinator approval request_id 6560378a-9871-43ec-94e1-ccd437a1727a. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 4 + CONFORMANCE.json | 15 +- CONFORMANCE.md | 3 +- .../manifests/openapm-v0.1.requirements.yml | 5 + docs/src/content/docs/specs/openapm-v0.1.md | 42 ++++- tests/spec_conformance/test_manifest_reqs.py | 167 +++++++++++++++++- 6 files changed, 227 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 946fe4538a..e27f1a602e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- `docs/src/content/docs/specs/openapm-v0.1.md` adds `req-tg-015`, documenting the already-shipped Codex-native `model`/`model_reasoning_effort` preservation and bounded dropped-metadata diagnostic as a normative conformance requirement. (#3150) + ### Fixed - Codex agent conversion preserves native `model` and `model_reasoning_effort` settings and warns about dropped metadata instead of silently losing it. (#3150) diff --git a/CONFORMANCE.json b/CONFORMANCE.json index 36753bd0bc..c5e719ae25 100644 --- a/CONFORMANCE.json +++ b/CONFORMANCE.json @@ -1544,12 +1544,23 @@ "tests": [ "tests/spec_conformance/test_manifest_reqs.py::test_user_scoped_mcp_target_selection_ignores_project_signals" ] + }, + { + "conformance_class": "consumer", + "id": "req-tg-015", + "keyword": "MUST", + "section": "8.5.1", + "status": "active", + "test_count": 1, + "tests": [ + "tests/spec_conformance/test_manifest_reqs.py::test_codex_native_model_settings_preserved_and_dropped_metadata_bounded" + ] } ], "spec_version": "v0.1.1", "summary_by_class": { "consumer": { - "active": 91, + "active": 92, "skipped": 1, "unbound": 0, "xfail": 0 @@ -1573,5 +1584,5 @@ "xfail": 0 } }, - "total_requirements": 125 + "total_requirements": 126 } diff --git a/CONFORMANCE.md b/CONFORMANCE.md index 2e145b8f97..79add1ad26 100644 --- a/CONFORMANCE.md +++ b/CONFORMANCE.md @@ -33,7 +33,7 @@ APM claims the proposed deployed-prompt-audit capability (req-pl-019, req-pl-020 | Class | Active | Skipped | Xfail | Unbound | |-------|-------:|--------:|------:|--------:| | Producer | 12 | 0 | 0 | 0 | -| Consumer | 91 | 1 | 0 | 0 | +| Consumer | 92 | 1 | 0 | 0 | | Registry | 1 | 0 | 0 | 0 | | Governance | 20 | 0 | 0 | 0 | @@ -166,6 +166,7 @@ APM claims the proposed deployed-prompt-audit capability (req-pl-019, req-pl-020 | [req-tg-012](docs/src/content/docs/specs/openapm-v0.1.md#req-tg-012) | MUST | 8.5.6 | consumer | active | 1 | - | | [req-tg-013](docs/src/content/docs/specs/openapm-v0.1.md#req-tg-013) | MUST | 8.5.7 | consumer | active | 7 | - | | [req-tg-014](docs/src/content/docs/specs/openapm-v0.1.md#req-tg-014) | MUST | 8.5.8 | consumer | active | 1 | - | +| [req-tg-015](docs/src/content/docs/specs/openapm-v0.1.md#req-tg-015) | MUST | 8.5.1 | consumer | active | 1 | - | ## Waivers diff --git a/docs/public/specs/manifests/openapm-v0.1.requirements.yml b/docs/public/specs/manifests/openapm-v0.1.requirements.yml index e67159dcb0..8ca6154b69 100644 --- a/docs/public/specs/manifests/openapm-v0.1.requirements.yml +++ b/docs/public/specs/manifests/openapm-v0.1.requirements.yml @@ -460,6 +460,11 @@ requirements: section: "8.5.8" conformance_class: consumer notes: "user-scoped MCP target selection uses a disclosed versioned capability contract, ignores project-only detection signals, applies first-source precedence without fallback, filters mixed sets, and refuses zero-supported selections before user manifest, lockfile, or target-config mutation" + - id: req-tg-015 + keyword: MUST + section: "8.5.1" + conformance_class: consumer + notes: "a consumer providing the accepted Codex-native agent conversion capability preserves a source-declared model or model_reasoning_effort string value in its native top-level placement, leaves either field absent when absent from the source, and emits a diagnostic bounded on both dropped-field count and per-field name length (never the value) for any other dropped, non-capability-restriction frontmatter field" - id: req-pr-006 keyword: MUST section: "8.1" diff --git a/docs/src/content/docs/specs/openapm-v0.1.md b/docs/src/content/docs/specs/openapm-v0.1.md index 64a3f0321b..eaae1f9853 100644 --- a/docs/src/content/docs/specs/openapm-v0.1.md +++ b/docs/src/content/docs/specs/openapm-v0.1.md @@ -136,7 +136,7 @@ between the companion corpus and the implementation. ### 1.3 Document conventions -- OpenAPM v0.1 carries **125 normative statements (120 MUST, 5 SHOULD)** indexed in +- OpenAPM v0.1 carries **126 normative statements (121 MUST, 5 SHOULD)** indexed in [Appendix C](#appendix-c-index-of-normative-statements). - All on-disk files defined by this specification are **YAML 1.2** parsed under the safe subset defined in @@ -2685,6 +2685,36 @@ intersection are not subject to this gate. > until then, conformance testing is scoped to the sets published in the > companion. + +**[req-tg-015]** A consumer implementation providing the accepted +Codex-native agent conversion capability MUST preserve a +source-declared `model` or `model_reasoning_effort` string value in +its native top-level placement in the generated Codex agent file, and +MUST leave either field absent from the generated file when it is +absent from the source. This clause does not require or imply +preservation of any other `config.toml`-native key, and does not +define behavior for any other conversion target. + +When that same consumer, converting to the Codex-native format, drops +a declared frontmatter field that is neither a +[req-tg-006](#req-tg-006) capability restriction nor one of the two +fields named above, it MUST emit a diagnostic naming the source agent, +and MUST bound both (a) the number of dropped field names it names +explicitly, and (b) the displayed length of each named field name, +each to a finite, implementation-documented limit; the diagnostic MUST +NOT include the dropped field's value. [req-tg-006](#req-tg-006)'s +existing capability-restriction diagnostic is unaffected and continues +to apply on its own terms. + +> **Editorial note.** `model` and `model_reasoning_effort` select a +> model or reasoning-effort level, not a tool, action, or resource +> ceiling, so they fall outside [req-tg-006](#req-tg-006)'s +> [capability-restriction](#3-terminology) scope and its +> widening/narrowing diagnostic framing. This requirement is scoped to +> consumers that already provide Codex-native agent conversion; it does +> not standardize dropped-metadata handling for any other target, and +> does not introduce a new vendor-namespaced metadata system. + #### 8.5.2 Post-install compilation guidance @@ -2953,8 +2983,8 @@ without a spec revision. The current matrix is in the companion [req-tg-008](#req-tg-008), [req-tg-009](#req-tg-009), [req-tg-010](#req-tg-010), [req-tg-011](#req-tg-011), [req-tg-012](#req-tg-012), [req-tg-013](#req-tg-013), - [req-tg-014](#req-tg-014), [req-pr-006](#req-pr-006), - [req-pr-007](#req-pr-007). + [req-tg-014](#req-tg-014), [req-tg-015](#req-tg-015), + [req-pr-006](#req-pr-006), [req-pr-007](#req-pr-007). --- @@ -3595,7 +3625,7 @@ conformance statement identifying: [req-tg-008](#req-tg-008), [req-tg-009](#req-tg-009), [req-tg-010](#req-tg-010), [req-tg-011](#req-tg-011), [req-tg-012](#req-tg-012), [req-tg-013](#req-tg-013), -[req-tg-014](#req-tg-014), +[req-tg-014](#req-tg-014), [req-tg-015](#req-tg-015), [req-sc-001](#req-sc-001), [req-sc-002](#req-sc-002), [req-sc-003](#req-sc-003), [req-sc-004](#req-sc-004), [req-sc-005](#req-sc-005), @@ -4058,6 +4088,7 @@ renumbering of conformance classes. | [req-tg-012](#req-tg-012) | MUST | 8.5.6 | consumer | | [req-tg-013](#req-tg-013) | MUST | 8.5.7 | consumer | | [req-tg-014](#req-tg-014) | MUST | 8.5.8 | consumer | +| [req-tg-015](#req-tg-015) | MUST | 8.5.1 | consumer | | [req-sc-001](#req-sc-001) | MUST | 10.4 | consumer | | [req-sc-002](#req-sc-002) | MUST | 10.9 | consumer | | [req-sc-003](#req-sc-003) | MUST | 10.3 | consumer | @@ -4077,7 +4108,7 @@ renumbering of conformance classes. | [req-cf-001](#req-cf-001) | MUST | 12.5 | consumer | | [req-cf-002](#req-cf-002) | MUST | 12.3 | consumer | -**Total normative statements: 125** (120 MUST, 5 SHOULD). +**Total normative statements: 126** (121 MUST, 5 SHOULD). --- @@ -4129,6 +4160,7 @@ renumbering of conformance classes. | 0.1.40 | 2026-09-07 | Spec-citation fold for dependency-policy identity casing in PR #2706. Added [req-pl-018] (Section 6.3.1, governance MUST) and extended [req-rs-016] clause (3): dependency allow, deny, and exact require operands use the documented per-host repository case rule, while registry-sourced repository coordinates are case-insensitive regardless of host; case normalization is ASCII-only, is bounded identically on both operands, stops at recursive-glob ambiguity, and does not cross virtual-path, ref, registry-name, MCP-name, unmanaged-path, or case-sensitive host/source boundaries; deny precedence is unchanged. Defined the policy glob grammar, documented byte-exact Section 6.4 merge behavior, and added the threat mapping. Classified this as a non-breaking correction of previously unspecified evaluation behavior under Section 9.2: existing lowercase workarounds remain matching; on registry sources and hosts documented as case-insensitive, case-variant allow entries can newly match, deny entries can newly enforce, and exact require entries can newly be satisfied, so those policies should be re-audited. Sections 1.3, 6.3.1, 6.3.5, 6.4, 6.5, 6.9, 7.2, 9.2, 10.8, 10.11, 11.2, and 11.3.4, Appendix C, and conformance coverage updated. Statement count: 121 -> 122 (117 MUST, 5 SHOULD). | | 0.1.41 | 2026-09-09 | Alias containment and lock-replay contract for PR #2901. Added [req-mf-025] (Section 4.3.2, consumer MUST), the optional lock-entry `alias` field, and conformance coverage. Under Section 9.2 this is an additive optional field and a defensive definition of previously unspecified alias behavior, not behavior-neutral errata: unsafe or reserved aliases can newly fail; valid dotted aliases remain accepted; surrounding whitespace is canonicalized; recorded aliases determine replay placement; absent aliases retain the unaliased layout. Source identity and permitted local source paths are unchanged. Older readers preserving the unknown field do not thereby implement placement support. Selects distinct 0.1.41 schema publication identities without changing published v0.1 URLs or bytes; Section 9.3 remains pending (see Appendix A). Sections 1.3, 4.9, 5.2, 10.7, 10.11, 11.3.2, and Appendix C updated. Statement count: 122 -> 123 (118 MUST, 5 SHOULD). | | 0.1.42 (proposed) | 2026-09-30 | Optional deployed-prompt audit capability for PR #2962. Added conditional governance [req-pl-019] and [req-pl-020] in Section 6.8.1, both enumerations, Appendix C, requirements manifest and behavioral conformance coverage. Under Section 9.2 this is a new opt-in conformance capability, not behavior-neutral errata or a reinterpretation of an existing obligation: implementations not claiming it acquire no new required feature. APM claims it; newly discovered prompt findings and incomplete native coverage can newly fail default and CI audits, command-only content remains non-failing, and protected remediation is refused before writes. No schema, lockfile version, existing mandatory feature, drift-policy or ownership rule changes. Section 9.3 reviewer approvals and public comment period remain pending; this proposal is not evidence of adoption. Statement count: 123 -> 125 (120 MUST, 5 SHOULD). | +| 0.1.43 (proposed) | 2026-10-03 | Codex-native agent model preservation and bounded dropped-metadata diagnostic for PR #3150 (closes #3126). Added [req-tg-015] (Section 8.5.1, consumer MUST): a consumer providing the accepted Codex-native agent conversion capability MUST preserve a source-declared `model` or `model_reasoning_effort` string value in its native top-level placement, MUST leave either field absent when absent from the source, and MUST emit a diagnostic bounded on both dropped-field count and per-field name length (never the value) for any other dropped, non-capability-restriction frontmatter field. Under Section 9.2 this is a new opt-in conformance capability scoped to the Codex-native conversion path, not behavior-neutral errata: it does not require or imply preservation of any other `config.toml`-native key and does not define behavior for any other conversion target. No schema, lockfile version, existing mandatory feature, drift-policy or ownership rule changes. Section 9.3 reviewer approvals and public comment period remain pending; this proposal is not evidence of adoption. Section 8.7, Section 11.3.2, and Appendix C updated. Statement count: 125 -> 126 (121 MUST, 5 SHOULD). | Errata (none at publication). diff --git a/tests/spec_conformance/test_manifest_reqs.py b/tests/spec_conformance/test_manifest_reqs.py index cbf9a9a8d2..4dec570afc 100644 --- a/tests/spec_conformance/test_manifest_reqs.py +++ b/tests/spec_conformance/test_manifest_reqs.py @@ -1,7 +1,7 @@ """Manifest (apm.yml) + scheme + tag + conformance-class tests. Covers req-mf-001..023, req-ext-001..002, req-sc-001..010, -req-tg-001..008, req-cf-001..002. +req-tg-001..009, req-tg-014..015, req-cf-001..002. Every requirement is exercised either by (a) schema validation against shipped fixtures (positive + negative), (b) a verbatim @@ -18,6 +18,7 @@ import jsonschema import pytest +import tomllib from apm_cli.adapters.client.base import MCPClientAdapter from apm_cli.deps.plugin_parser import normalize_plugin_directory @@ -33,6 +34,7 @@ CATEGORY_WARNING, DiagnosticCollector, ) +from apm_cli.utils.yaml_io import yaml_to_str from tests.spec_conformance._helpers import ( assert_spec_contains, load_json_fixture, @@ -1055,6 +1057,169 @@ def test_kiro_agent_tools_gate_fails_closed_before_adopt(tmp_path: Path) -> None ) +@pytest.mark.req("req-tg-015") +def test_codex_native_model_settings_preserved_and_dropped_metadata_bounded( + tmp_path: Path, +) -> None: + """req-tg-015: Codex model/effort preservation plus a bounded diagnostic. + + Exercises the actual generated Codex agent output (parsed TOML, not + string matching) for: both fields present, only one present, neither + present, a non-string value, and an unsupported non-capability field + that must be named in a diagnostic bounded on BOTH the number of named + keys and each key's displayed length -- never the value. Also confirms + req-tg-006's own tools-capability diagnostic and non-Codex target + deployment are unaffected by this clause. + """ + # Both fields present -> both reach the generated TOML verbatim. + both_source = tmp_path / "both.agent.md" + both_source.write_text( + f"---\n{yaml_to_str({'name': 'reviewer', 'model': 'gpt-5.6-sol', 'model_reasoning_effort': 'high'})}---\nReview changes.\n", + encoding="utf-8", + ) + both_target = tmp_path / "both.toml" + both_diagnostics = DiagnosticCollector() + AgentIntegrator._write_codex_agent( + both_source, both_target, diagnostics=both_diagnostics, package_name="spec-fixture" + ) + both_doc = tomllib.loads(both_target.read_text(encoding="utf-8")) + assert both_doc["model"] == "gpt-5.6-sol" + assert both_doc["model_reasoning_effort"] == "high" + assert both_diagnostics.by_category() == {} + + # Only one present -> the other key is absent (not null/empty) from the + # parsed TOML. + one_source = tmp_path / "one.agent.md" + one_source.write_text( + f"---\n{yaml_to_str({'name': 'reviewer', 'model': 'gpt-5.6-sol'})}---\nReview changes.\n", + encoding="utf-8", + ) + one_target = tmp_path / "one.toml" + AgentIntegrator._write_codex_agent( + one_source, one_target, diagnostics=DiagnosticCollector(), package_name="spec-fixture" + ) + one_doc = tomllib.loads(one_target.read_text(encoding="utf-8")) + assert one_doc["model"] == "gpt-5.6-sol" + assert "model_reasoning_effort" not in one_doc + + # Neither present -> neither key appears. + neither_source = tmp_path / "neither.agent.md" + neither_source.write_text( + f"---\n{yaml_to_str({'name': 'reviewer'})}---\nReview changes.\n", + encoding="utf-8", + ) + neither_target = tmp_path / "neither.toml" + AgentIntegrator._write_codex_agent( + neither_source, + neither_target, + diagnostics=DiagnosticCollector(), + package_name="spec-fixture", + ) + neither_doc = tomllib.loads(neither_target.read_text(encoding="utf-8")) + assert "model" not in neither_doc + assert "model_reasoning_effort" not in neither_doc + + # A non-string value -> dropped, with a bounded diagnostic naming the + # field (not its value). + bad_source = tmp_path / "bad.agent.md" + bad_source.write_text( + f"---\n{yaml_to_str({'name': 'reviewer', 'model_reasoning_effort': 7})}---\nReview changes.\n", + encoding="utf-8", + ) + bad_target = tmp_path / "bad.toml" + bad_diagnostics = DiagnosticCollector() + AgentIntegrator._write_codex_agent( + bad_source, bad_target, diagnostics=bad_diagnostics, package_name="spec-fixture" + ) + bad_doc = tomllib.loads(bad_target.read_text(encoding="utf-8")) + assert "model_reasoning_effort" not in bad_doc + bad_warnings = bad_diagnostics.by_category().get(CATEGORY_AGENT_LOSSY_COMPILATION, []) + assert len(bad_warnings) == 1 + assert "model_reasoning_effort" in bad_warnings[0].message + assert "must be a string" in bad_warnings[0].message + assert "7" not in bad_warnings[0].message + + # Unsupported non-capability metadata, both over the count bound (8 + # keys, one more than _MAX_DROPPED_FIELDS_SHOWN = 5) and with one + # individually oversized key name (20000 chars) -- the diagnostic must + # be bounded on BOTH axes and must never include a value. + oversized_key = "k" * 20_000 + unsupported = { + oversized_key: "do-not-leak-this-value", + "bad\x1b[31mkey": "do-not-leak-this-value-either", + "field_c": "value_c", + "field_d": "value_d", + "field_e": "value_e", + "field_f": "value_f", + "field_g": "value_g", + "field_h": "value_h", + } + meta_source = tmp_path / "meta.agent.md" + meta_source.write_text( + f"---\n{yaml_to_str({'name': 'reviewer', 'model': 'gpt-5.6-sol', **unsupported})}---\nReview changes.\n", + encoding="utf-8", + ) + meta_target = tmp_path / "meta.toml" + meta_diagnostics = DiagnosticCollector() + AgentIntegrator._write_codex_agent( + meta_source, meta_target, diagnostics=meta_diagnostics, package_name="spec-fixture" + ) + meta_doc = tomllib.loads(meta_target.read_text(encoding="utf-8")) + assert meta_doc["model"] == "gpt-5.6-sol" + assert all(field not in meta_doc for field in unsupported) + meta_warnings = meta_diagnostics.by_category().get(CATEGORY_AGENT_LOSSY_COMPILATION, []) + metadata_warning = next(w for w in meta_warnings if "not translated by APM" in w.message) + # Count bound: 8 unsupported keys supplied, only 5 named explicitly. + assert "(and 3 more)" in metadata_warning.message + # Per-key length bound: the raw 20000-char key never appears whole. + assert oversized_key not in metadata_warning.message + assert "...(truncated)" in metadata_warning.message + # No value ever leaks into the diagnostic. + for value in unsupported.values(): + assert value not in metadata_warning.message + # ASCII-safe even with a control-character key. + assert all(32 <= ord(c) <= 126 or c in "\n\r\t" for c in metadata_warning.message) + # Overall message length stays bounded despite the 20000-char input key. + assert len(metadata_warning.message) < 600 + + # req-tg-006's own capability-restriction diagnostic is unaffected: a + # dropped `tools` field still fires its own, separate warning. + tools_source = tmp_path / "tools.agent.md" + tools_source.write_text( + f"---\n{yaml_to_str({'name': 'reviewer', 'model': 'gpt-5.6-sol', 'tools': ['read']})}---\nReview changes.\n", + encoding="utf-8", + ) + tools_target = tmp_path / "tools.toml" + tools_diagnostics = DiagnosticCollector() + AgentIntegrator._write_codex_agent( + tools_source, tools_target, diagnostics=tools_diagnostics, package_name="spec-fixture" + ) + tools_doc = tomllib.loads(tools_target.read_text(encoding="utf-8")) + assert tools_doc["model"] == "gpt-5.6-sol" + tools_warnings = tools_diagnostics.by_category().get(CATEGORY_AGENT_LOSSY_COMPILATION, []) + assert any("field 'tools' was dropped" in w.message for w in tools_warnings) + + # A different (non-Codex) target is unaffected by this clause: the + # source agent is deployed verbatim, filtering logic does not run. + verbatim_source = tmp_path / "verbatim.agent.md" + verbatim_content = ( + "---\nname: reviewer\nmodel: native-model\nmodel_reasoning_effort: high\n" + "model_verbosity: low\n---\nReview changes.\n" + ) + verbatim_source.write_text(verbatim_content, encoding="utf-8") + assert verbatim_source.read_text(encoding="utf-8") == verbatim_content + + assert_spec_contains( + "A consumer implementation providing the accepted\nCodex-native agent conversion capability MUST preserve a\nsource-declared", + "MUST leave either field absent from the generated file when it is\nabsent from the source", + "This clause does not require or imply\npreservation of any other", + "does not\ndefine behavior for any other conversion target", + "it MUST emit a diagnostic naming the source agent,\nand MUST bound both", + "the diagnostic MUST\nNOT include the dropped field's value", + "existing capability-restriction diagnostic is unaffected and continues\nto apply on its own terms", + ) + + # --- req-cf-001..002 -------------------------------------------------- From fce1d9b8e9d2df09411535dc4ba8b441ce2f7211 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Sat, 3 Oct 2026 21:11:23 +0200 Subject: [PATCH 07/13] docs(spec): fold apm-spec-guardian round-1 findings into req-tg-015 Genuine 4-panelist fan-out (spec-swagger-editor, spec-oci-editor, spec-pkgmgr-editor, spec-tag-architect) + spec-editor-synthesizer reviewed req-tg-015 as modified by this PR. All four panels returned ship_with_followups, shocked_meter=8, zero blocking findings. Synthesizer ship_decision: fold_and_ship. Applied fold_now items (independently verified against shipped _write_codex_agent/_display_dropped_field_key behavior before folding -- zero src/** changes required or made): - F1: clarify that a non-string model/model_reasoning_effort value is not subject to preservation and the consumer MUST still diagnose it (reworded from the synthesizer's draft to avoid misdescribing the diagnostic as shared with the generic dropped-fields bucket; production code gives it its own distinct message). - F2: new MUST that each named dropped-field name is sanitized to printable ASCII before display (already true: _display_dropped_field_key calls printable_ascii_text before truncating). - F3: Appendix D 0.1.43 negative-scope sentence paralleling the 0.1.42 precedent ("implementations not providing Codex-native agent conversion acquire no new required feature"). - F4: new MUST that the named-field-name limit is at least one whenever at least one field was dropped (already true: _MAX_DROPPED_FIELDS_SHOWN = 5, always > 0). Deferred per synthesizer classification (to be documented in the advisory comment only, not applied here): F5 (SHOULD-level key-name redaction guidance), F6 (Appendix C re-sort), F7 (TOML-key parenthetical), F8 (Appendix D terminology alignment) -> v0.1.1; F9 (manifest schema `applicability` key, affects all req-tg-*) -> v0.2, reserved-slot-anchored to Section 9.2. Rejected: tag-rec-r1-1 (section-layering) -- editorial note already fences the concern sufficiently for this fold. Statement count unchanged (126/121 MUST/5 SHOULD): all four fold items land as additional sentences within the existing req-tg-015 anchor, not a new anchor. orphan_check confirms 126 aligned. Updated test_manifest_reqs.py assert_spec_contains needles to match the expanded req-tg-015 prose; no production/test-assertion logic changed (the already-shipped behavior this codifies was verified in the prior commit's test coverage). --- .../manifests/openapm-v0.1.requirements.yml | 2 +- docs/src/content/docs/specs/openapm-v0.1.md | 18 ++++++++++++------ tests/spec_conformance/test_manifest_reqs.py | 8 +++++--- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/docs/public/specs/manifests/openapm-v0.1.requirements.yml b/docs/public/specs/manifests/openapm-v0.1.requirements.yml index 8ca6154b69..3f8b072aaf 100644 --- a/docs/public/specs/manifests/openapm-v0.1.requirements.yml +++ b/docs/public/specs/manifests/openapm-v0.1.requirements.yml @@ -464,7 +464,7 @@ requirements: keyword: MUST section: "8.5.1" conformance_class: consumer - notes: "a consumer providing the accepted Codex-native agent conversion capability preserves a source-declared model or model_reasoning_effort string value in its native top-level placement, leaves either field absent when absent from the source, and emits a diagnostic bounded on both dropped-field count and per-field name length (never the value) for any other dropped, non-capability-restriction frontmatter field" + notes: "a consumer providing the accepted Codex-native agent conversion capability preserves a source-declared model or model_reasoning_effort string value in its native top-level placement, leaves either field absent when absent from the source, still diagnoses a non-string source value for either field, and emits a diagnostic bounded on dropped-field count (at least one shown when any are dropped), per-field name length, and ASCII-sanitized field-name display (never the value) for any other dropped, non-capability-restriction frontmatter field" - id: req-pr-006 keyword: MUST section: "8.1" diff --git a/docs/src/content/docs/specs/openapm-v0.1.md b/docs/src/content/docs/specs/openapm-v0.1.md index eaae1f9853..fd262bd4d1 100644 --- a/docs/src/content/docs/specs/openapm-v0.1.md +++ b/docs/src/content/docs/specs/openapm-v0.1.md @@ -2693,7 +2693,10 @@ its native top-level placement in the generated Codex agent file, and MUST leave either field absent from the generated file when it is absent from the source. This clause does not require or imply preservation of any other `config.toml`-native key, and does not -define behavior for any other conversion target. +define behavior for any other conversion target. A source-declared +`model` or `model_reasoning_effort` value that is not a YAML string +scalar is not subject to preservation under this clause, and the +consumer MUST still emit a diagnostic for it. When that same consumer, converting to the Codex-native format, drops a declared frontmatter field that is neither a @@ -2701,10 +2704,13 @@ a declared frontmatter field that is neither a fields named above, it MUST emit a diagnostic naming the source agent, and MUST bound both (a) the number of dropped field names it names explicitly, and (b) the displayed length of each named field name, -each to a finite, implementation-documented limit; the diagnostic MUST -NOT include the dropped field's value. [req-tg-006](#req-tg-006)'s -existing capability-restriction diagnostic is unaffected and continues -to apply on its own terms. +each to a finite, implementation-documented limit (the named-field-name +limit MUST be at least one when at least one field was dropped); each +named field name MUST be sanitized to printable ASCII (U+0020 through +U+007E), replacing or escaping any byte outside that range before +display; the diagnostic MUST NOT include the dropped field's value. +[req-tg-006](#req-tg-006)'s existing capability-restriction diagnostic +is unaffected and continues to apply on its own terms. > **Editorial note.** `model` and `model_reasoning_effort` select a > model or reasoning-effort level, not a tool, action, or resource @@ -4160,7 +4166,7 @@ renumbering of conformance classes. | 0.1.40 | 2026-09-07 | Spec-citation fold for dependency-policy identity casing in PR #2706. Added [req-pl-018] (Section 6.3.1, governance MUST) and extended [req-rs-016] clause (3): dependency allow, deny, and exact require operands use the documented per-host repository case rule, while registry-sourced repository coordinates are case-insensitive regardless of host; case normalization is ASCII-only, is bounded identically on both operands, stops at recursive-glob ambiguity, and does not cross virtual-path, ref, registry-name, MCP-name, unmanaged-path, or case-sensitive host/source boundaries; deny precedence is unchanged. Defined the policy glob grammar, documented byte-exact Section 6.4 merge behavior, and added the threat mapping. Classified this as a non-breaking correction of previously unspecified evaluation behavior under Section 9.2: existing lowercase workarounds remain matching; on registry sources and hosts documented as case-insensitive, case-variant allow entries can newly match, deny entries can newly enforce, and exact require entries can newly be satisfied, so those policies should be re-audited. Sections 1.3, 6.3.1, 6.3.5, 6.4, 6.5, 6.9, 7.2, 9.2, 10.8, 10.11, 11.2, and 11.3.4, Appendix C, and conformance coverage updated. Statement count: 121 -> 122 (117 MUST, 5 SHOULD). | | 0.1.41 | 2026-09-09 | Alias containment and lock-replay contract for PR #2901. Added [req-mf-025] (Section 4.3.2, consumer MUST), the optional lock-entry `alias` field, and conformance coverage. Under Section 9.2 this is an additive optional field and a defensive definition of previously unspecified alias behavior, not behavior-neutral errata: unsafe or reserved aliases can newly fail; valid dotted aliases remain accepted; surrounding whitespace is canonicalized; recorded aliases determine replay placement; absent aliases retain the unaliased layout. Source identity and permitted local source paths are unchanged. Older readers preserving the unknown field do not thereby implement placement support. Selects distinct 0.1.41 schema publication identities without changing published v0.1 URLs or bytes; Section 9.3 remains pending (see Appendix A). Sections 1.3, 4.9, 5.2, 10.7, 10.11, 11.3.2, and Appendix C updated. Statement count: 122 -> 123 (118 MUST, 5 SHOULD). | | 0.1.42 (proposed) | 2026-09-30 | Optional deployed-prompt audit capability for PR #2962. Added conditional governance [req-pl-019] and [req-pl-020] in Section 6.8.1, both enumerations, Appendix C, requirements manifest and behavioral conformance coverage. Under Section 9.2 this is a new opt-in conformance capability, not behavior-neutral errata or a reinterpretation of an existing obligation: implementations not claiming it acquire no new required feature. APM claims it; newly discovered prompt findings and incomplete native coverage can newly fail default and CI audits, command-only content remains non-failing, and protected remediation is refused before writes. No schema, lockfile version, existing mandatory feature, drift-policy or ownership rule changes. Section 9.3 reviewer approvals and public comment period remain pending; this proposal is not evidence of adoption. Statement count: 123 -> 125 (120 MUST, 5 SHOULD). | -| 0.1.43 (proposed) | 2026-10-03 | Codex-native agent model preservation and bounded dropped-metadata diagnostic for PR #3150 (closes #3126). Added [req-tg-015] (Section 8.5.1, consumer MUST): a consumer providing the accepted Codex-native agent conversion capability MUST preserve a source-declared `model` or `model_reasoning_effort` string value in its native top-level placement, MUST leave either field absent when absent from the source, and MUST emit a diagnostic bounded on both dropped-field count and per-field name length (never the value) for any other dropped, non-capability-restriction frontmatter field. Under Section 9.2 this is a new opt-in conformance capability scoped to the Codex-native conversion path, not behavior-neutral errata: it does not require or imply preservation of any other `config.toml`-native key and does not define behavior for any other conversion target. No schema, lockfile version, existing mandatory feature, drift-policy or ownership rule changes. Section 9.3 reviewer approvals and public comment period remain pending; this proposal is not evidence of adoption. Section 8.7, Section 11.3.2, and Appendix C updated. Statement count: 125 -> 126 (121 MUST, 5 SHOULD). | +| 0.1.43 (proposed) | 2026-10-03 | Codex-native agent model preservation and bounded dropped-metadata diagnostic for PR #3150 (closes #3126). Added [req-tg-015] (Section 8.5.1, consumer MUST): a consumer providing the accepted Codex-native agent conversion capability MUST preserve a source-declared `model` or `model_reasoning_effort` string value in its native top-level placement, MUST leave either field absent when absent from the source, and MUST emit a diagnostic bounded on both dropped-field count and per-field name length (never the value) for any other dropped, non-capability-restriction frontmatter field. Under Section 9.2 this is a new opt-in conformance capability scoped to the Codex-native conversion path, not behavior-neutral errata: it does not require or imply preservation of any other `config.toml`-native key and does not define behavior for any other conversion target. Implementations not providing Codex-native agent conversion acquire no new required feature. No schema, lockfile version, existing mandatory feature, drift-policy or ownership rule changes. Section 9.3 reviewer approvals and public comment period remain pending; this proposal is not evidence of adoption. Section 8.7, Section 11.3.2, and Appendix C updated. Statement count: 125 -> 126 (121 MUST, 5 SHOULD). | Errata (none at publication). diff --git a/tests/spec_conformance/test_manifest_reqs.py b/tests/spec_conformance/test_manifest_reqs.py index 4dec570afc..66b90d31c3 100644 --- a/tests/spec_conformance/test_manifest_reqs.py +++ b/tests/spec_conformance/test_manifest_reqs.py @@ -1213,10 +1213,12 @@ def test_codex_native_model_settings_preserved_and_dropped_metadata_bounded( "A consumer implementation providing the accepted\nCodex-native agent conversion capability MUST preserve a\nsource-declared", "MUST leave either field absent from the generated file when it is\nabsent from the source", "This clause does not require or imply\npreservation of any other", - "does not\ndefine behavior for any other conversion target", + "define behavior for any other conversion target. A source-declared\n`model` or `model_reasoning_effort` value that is not a YAML string\nscalar is not subject to preservation under this clause, and the\nconsumer MUST still emit a diagnostic for it.", "it MUST emit a diagnostic naming the source agent,\nand MUST bound both", - "the diagnostic MUST\nNOT include the dropped field's value", - "existing capability-restriction diagnostic is unaffected and continues\nto apply on its own terms", + "limit (the named-field-name\nlimit MUST be at least one when at least one field was dropped)", + "each\nnamed field name MUST be sanitized to printable ASCII (U+0020 through\nU+007E), replacing or escaping any byte outside that range before\ndisplay", + "the diagnostic MUST NOT include the dropped field's value", + "existing capability-restriction diagnostic\nis unaffected and continues to apply on its own terms", ) From fc652ea9f4b709a120cc635b1844ce017f0e1f7a Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Sat, 3 Oct 2026 21:25:12 +0200 Subject: [PATCH 08/13] test(spec-conformance): exercise real non-Codex deployment for req-tg-015 The non-Codex-target assertion in test_codex_native_model_settings_preserved_and_dropped_metadata_bounded previously only wrote a fixture file and re-read it back verbatim, never invoking integrate_agents_for_target or any deployment path. That made the claimed non-Codex-unaffected coverage vacuous (0 calls to the deployment entrypoint). Replace it with a real call to AgentIntegrator().integrate_agents_for_target against the 'claude' target (which deploys via copy_agent, a verbatim copy with zero frontmatter filtering), asserting the deployed file content is byte-identical to the source including model/ model_reasoning_effort/model_verbosity, and that no req-tg-015 lossy-compilation diagnostic fires for this target. Mutation-tested: disabling integrate_agents_for_target's dispatch body makes the new assertion fail (0 != 1); restored and reverified passing. Test-only change; no src/** modified. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/spec_conformance/test_manifest_reqs.py | 60 ++++++++++++++++++-- 1 file changed, 56 insertions(+), 4 deletions(-) diff --git a/tests/spec_conformance/test_manifest_reqs.py b/tests/spec_conformance/test_manifest_reqs.py index 66b90d31c3..d291801692 100644 --- a/tests/spec_conformance/test_manifest_reqs.py +++ b/tests/spec_conformance/test_manifest_reqs.py @@ -1199,15 +1199,67 @@ def test_codex_native_model_settings_preserved_and_dropped_metadata_bounded( tools_warnings = tools_diagnostics.by_category().get(CATEGORY_AGENT_LOSSY_COMPILATION, []) assert any("field 'tools' was dropped" in w.message for w in tools_warnings) - # A different (non-Codex) target is unaffected by this clause: the - # source agent is deployed verbatim, filtering logic does not run. - verbatim_source = tmp_path / "verbatim.agent.md" + # A different (non-Codex) target is unaffected by this clause: deploy + # through the real integration dispatch (not a write+reread of the + # source alone) and confirm the target file carries the frontmatter + # verbatim, including `model`/`model_reasoning_effort`/the unsupported + # `model_verbosity` key, because non-Codex/non-Kiro targets copy the + # agent through `copy_agent` with no field-level filtering. + from datetime import datetime + + from apm_cli.integration.targets import KNOWN_TARGETS + from apm_cli.models.apm_package import ( + APMPackage, + GitReferenceType, + PackageInfo, + ResolvedReference, + ) + + nc_project_root = tmp_path / "nc_project" + (nc_project_root / ".claude").mkdir(parents=True) + nc_package_dir = tmp_path / "nc_pkg" + nc_apm_agents = nc_package_dir / ".apm" / "agents" + nc_apm_agents.mkdir(parents=True) verbatim_content = ( "---\nname: reviewer\nmodel: native-model\nmodel_reasoning_effort: high\n" "model_verbosity: low\n---\nReview changes.\n" ) + verbatim_source = nc_apm_agents / "verbatim.agent.md" verbatim_source.write_text(verbatim_content, encoding="utf-8") - assert verbatim_source.read_text(encoding="utf-8") == verbatim_content + + nc_package = APMPackage( + name="nc-pkg", + version="1.0.0", + package_path=nc_package_dir, + source="github.com/test/nc-pkg", + ) + nc_resolved_ref = ResolvedReference( + original_ref="main", + ref_type=GitReferenceType.BRANCH, + resolved_commit="abc123", + ref_name="main", + ) + nc_pi = PackageInfo( + package=nc_package, + install_path=nc_package_dir, + resolved_reference=nc_resolved_ref, + installed_at=datetime.now().isoformat(), + ) + + nc_diagnostics = DiagnosticCollector() + nc_result = AgentIntegrator().integrate_agents_for_target( + KNOWN_TARGETS["claude"], + nc_pi, + nc_project_root, + diagnostics=nc_diagnostics, + ) + assert nc_result.files_integrated == 1 + nc_target_path = nc_project_root / ".claude" / "agents" / "verbatim.md" + assert nc_target_path.read_text(encoding="utf-8") == verbatim_content + # No req-tg-015 diagnostic fires for this target: the dropped-field + # bounding/preservation clause is Codex-specific and copy_agent never + # inspects frontmatter keys. + assert nc_diagnostics.by_category().get(CATEGORY_AGENT_LOSSY_COMPILATION, []) == [] assert_spec_contains( "A consumer implementation providing the accepted\nCodex-native agent conversion capability MUST preserve a\nsource-declared", From a875a7712212134b3acc2d34636169d76f97bb77 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 5 Oct 2026 11:34:15 +0200 Subject: [PATCH 09/13] fix(apm-spec-guardian): repair Draft7 allOf/reserved_slot_anchor contradiction The synthesizer-return schema's defer_v0_2.items used allOf to compose fold_item with an added reserved_slot_anchor property, but fold_item's own additionalProperties:false rejected the valid defer_v0_2 return before the second allOf branch could add it back -- a closed composition that always failed Draft 7 validation for a conforming defer_v0_2 entry (false rejection, not false acceptance). Fix: inline fold_item's 5 properties plus reserved_slot_anchor into a new fold_item_with_reserved_slot definition with a single additionalProperties:false/required over all 6 keys, and reference it directly from defer_v0_2.items instead of composing via allOf. fold_item itself (used by fold_now/defer_v0_1_1) is untouched. Add regression coverage: Draft7 meta-validity, before/after proof (reconstructed pre-fix schema fails the real original synthesizer return and a valid synthetic report; the fixed schema passes both), load-bearing negative-guard mutation tests for the missing-anchor and additionalProperties guards, unknown-property rejection across fold_now/defer_v0_1_1/defer_v0_2 and at the top level, invalid-type and malformed-id/shocked_meter rejection, a structural-parity check between fold_item and fold_item_with_reserved_slot, and a byte-identity check between the canonical schema and its deployed .agents/ mirror. Add the genuine retained PR #3150 synthesizer return (round 1, containing finding F9) as a fixture for the before/after proof. Document in the contributing guide that reserved_slot_anchor is a structural (type-only) schema check, not semantic verification that a cited anchor is one of the spec's actual reserved-for-v0.2 sections -- that judgment remains human/panel work. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../assets/synthesizer-return-schema.json | 33 +- .../docs/contributing/development-guide.md | 10 + .../pr3150-synthesizer-return-original.json | 145 ++++++++ ...st_apm_spec_guardian_synthesizer_schema.py | 312 ++++++++++++++++++ 4 files changed, 485 insertions(+), 15 deletions(-) create mode 100644 tests/fixtures/spec-guardian/pr3150-synthesizer-return-original.json create mode 100644 tests/unit/test_apm_spec_guardian_synthesizer_schema.py diff --git a/.apm/skills/apm-spec-guardian/assets/synthesizer-return-schema.json b/.apm/skills/apm-spec-guardian/assets/synthesizer-return-schema.json index 7c55b591bc..a72d177b88 100644 --- a/.apm/skills/apm-spec-guardian/assets/synthesizer-return-schema.json +++ b/.apm/skills/apm-spec-guardian/assets/synthesizer-return-schema.json @@ -93,21 +93,7 @@ "defer_v0_2": { "type": "array", "description": "Architectural work bound to a reserved slot in a future major. Each entry MUST cite the existing reserved-slot section anchor in the artifact (e.g. 'sec.4.8 workspaces reserved', 'sec.10.12 publisher attestations reserved').", - "items": { - "allOf": [ - { "$ref": "#/definitions/fold_item" }, - { - "type": "object", - "required": ["reserved_slot_anchor"], - "properties": { - "reserved_slot_anchor": { - "type": "string", - "description": "Section / anchor in the current artifact that already reserves space for this work." - } - } - } - ] - } + "items": { "$ref": "#/definitions/fold_item_with_reserved_slot" } }, "reject": { "type": "array", @@ -148,6 +134,23 @@ "patch_instruction": { "type": "string", "description": "Concrete mechanical instruction the drafter applies. ASCII only. MUST NOT add any forbidden vendor / foundation token to the artifact." }, "success_criterion": { "type": "string", "description": "Verifiable post-fold check (grep, schema validation, or anchor count)." } } + }, + "fold_item_with_reserved_slot": { + "type": "object", + "description": "Same shape as fold_item plus a required reserved_slot_anchor. Inlined (not composed via allOf over fold_item) because Draft 7 has no unevaluatedProperties: an allOf branch's own additionalProperties:false would otherwise reject the sibling branch's reserved_slot_anchor key. The five base properties are intentionally duplicated here, not $ref-shared, to keep this a single closed object; test_shared_fold_item_properties_structurally_identical guards against the two copies drifting apart.", + "required": ["id", "theme", "spec_section", "patch_instruction", "success_criterion", "reserved_slot_anchor"], + "additionalProperties": false, + "properties": { + "id": { "type": "string", "pattern": "^F[1-9][0-9]*$" }, + "theme": { "type": "string", "description": "Convergent-theme id this fold addresses (e.g. T1, T2), or 'standalone' if not part of a clustered theme." }, + "spec_section": { "type": "string", "description": "Section / appendix / req-XXX anchor where the patch lands." }, + "patch_instruction": { "type": "string", "description": "Concrete mechanical instruction the drafter applies. ASCII only. MUST NOT add any forbidden vendor / foundation token to the artifact." }, + "success_criterion": { "type": "string", "description": "Verifiable post-fold check (grep, schema validation, or anchor count)." }, + "reserved_slot_anchor": { + "type": "string", + "description": "Section / anchor in the current artifact that already reserves space for this work." + } + } } } } diff --git a/docs/src/content/docs/contributing/development-guide.md b/docs/src/content/docs/contributing/development-guide.md index ab1f83cdc8..c0c9e2e80f 100644 --- a/docs/src/content/docs/contributing/development-guide.md +++ b/docs/src/content/docs/contributing/development-guide.md @@ -500,3 +500,13 @@ The workflow distinguishes three modes: These checks cannot detect every semantic drift. Choosing the appropriate mode remains a human decision; the harness exposes the choice, not the answer. + +The same limit applies to the `apm-spec-guardian` skill's review panel. Its +synthesizer-return schema validates that a `defer_v0_2` entry's +`reserved_slot_anchor` field is **present and of type string** -- a +structural, type-only check. It does not verify that the cited anchor is one of +the spec's actual reserved-for-v0.2 sections (for example Section 4.8 +workspaces, Section 7.9 version withdrawal, Section 10.12 publisher +attestations, or Appendix B). Confirming that a cited anchor genuinely +reserves the space it claims remains a human/panel judgment, not something +schema validation can substitute for. diff --git a/tests/fixtures/spec-guardian/pr3150-synthesizer-return-original.json b/tests/fixtures/spec-guardian/pr3150-synthesizer-return-original.json new file mode 100644 index 0000000000..26d91b8a94 --- /dev/null +++ b/tests/fixtures/spec-guardian/pr3150-synthesizer-return-original.json @@ -0,0 +1,145 @@ +{ + "round": 1, + "shocked_meter_avg": 8.0, + "convergence_table": [ + { + "panel": "spec-swagger-editor", + "verdict": "ship_with_followups", + "shocked_meter": 8, + "new_blockers": 0, + "new_recommended": 2, + "new_nits": 2 + }, + { + "panel": "spec-oci-editor", + "verdict": "ship_with_followups", + "shocked_meter": 8, + "new_blockers": 0, + "new_recommended": 2, + "new_nits": 2 + }, + { + "panel": "spec-pkgmgr-editor", + "verdict": "ship_with_followups", + "shocked_meter": 8, + "new_blockers": 0, + "new_recommended": 1, + "new_nits": 1 + }, + { + "panel": "spec-tag-architect", + "verdict": "ship_with_followups", + "shocked_meter": 8, + "new_blockers": 0, + "new_recommended": 2, + "new_nits": 2 + } + ], + "convergent_themes": [ + { + "id": "T1", + "label": "Non-string source-value fall-through for model/model_reasoning_effort is unspecified", + "supporting_finding_ids": [ + "sw-rec-r1-2", + "oci-nit-r1-1" + ] + }, + { + "id": "T2", + "label": "Appendix C section-number ordering disrupted by id-ordered req-tg-015 placement", + "supporting_finding_ids": [ + "sw-nit-r1-2", + "oci-nit-r1-2", + "pkg-nit-r1-1", + "tag-nit-r1-1" + ] + }, + { + "id": "T3", + "label": "Diagnostic field names are attacker-controlled and need sanitization/redaction guarantees", + "supporting_finding_ids": [ + "oci-rec-r1-1", + "oci-rec-r1-2" + ] + } + ], + "fold_now": [ + { + "id": "F1", + "theme": "T1", + "spec_section": "req-tg-015", + "patch_instruction": "After the sentence ending '...in its native top-level placement in the generated Codex agent file.' append a new non-normative sentence: 'A source-declared model or model_reasoning_effort value that is not a YAML string scalar is not subject to preservation under this clause and is handled as any other non-capability-restriction frontmatter field.'", + "success_criterion": "grep the req-tg-015 block for the phrase 'not a YAML string scalar'; expect exactly one match." + }, + { + "id": "F2", + "theme": "T3", + "spec_section": "req-tg-015", + "patch_instruction": "In the bounded-diagnostic paragraph, after 'each to a finite, implementation-documented limit' insert: '; field names rendered in the diagnostic MUST be sanitized to printable ASCII (U+0020 through U+007E), replacing or escaping any byte outside that range before display'. This codifies behavior already implemented by the reference CLI's printable_ascii_text utility.", + "success_criterion": "grep req-tg-015 block for 'printable ASCII (U+0020 through U+007E)'; expect exactly one match. Statement count increments by 1 MUST (126->127 total, 122 MUST + 5 SHOULD); update Section 1.3, Appendix C footer, and Appendix D 0.1.43 row accordingly." + }, + { + "id": "F3", + "theme": "standalone", + "spec_section": "Appendix D, row 0.1.43", + "patch_instruction": "Append to the end of the 0.1.43 Appendix D revision entry, after '...does not define behavior for any other conversion target.': ' Implementations not providing Codex-native agent conversion acquire no new required feature.' This mirrors the negative-scope framing established in the 0.1.42 entry.", + "success_criterion": "grep the 0.1.43 Appendix D row for 'acquire no new required feature'; expect exactly one match." + }, + { + "id": "F4", + "theme": "standalone", + "spec_section": "req-tg-015", + "patch_instruction": "After 'a finite, implementation-documented limit' (in the count-bounding clause for dropped field names), insert a parenthetical: '(which MUST be at least one when at least one field was dropped)' to establish a non-vacuous floor. This codifies the reference CLI's _MAX_DROPPED_FIELDS_SHOWN = 5, which already satisfies this floor.", + "success_criterion": "grep req-tg-015 for 'at least one when at least one field was dropped'; expect exactly one match. If F2 also lands, total statement count is 127->128 (123 MUST + 5 SHOULD); reconcile Section 1.3, Appendix C footer, and Appendix D accordingly." + } + ], + "defer_v0_1_1": [ + { + "id": "F5", + "theme": "T3", + "spec_section": "req-tg-015", + "patch_instruction": "Add a non-normative editorial note after the diagnostic paragraph: 'Implementations SHOULD treat rendered field names as potentially attacker-controlled and apply the same redaction or sanitization policy they apply to other user-supplied identifiers in diagnostic output.' This is a SHOULD-level guidance note building on the MUST sanitization from F2.", + "success_criterion": "grep req-tg-015 editorial notes for 'attacker-controlled'; expect exactly one match." + }, + { + "id": "F6", + "theme": "T2", + "spec_section": "Appendix C table", + "patch_instruction": "Re-sort the Appendix C table rows so that req-tg-015 (Section 8.5.1) sits between req-tg-006 (8.5.1) and req-tg-007 (8.5.2), restoring section-number ordering within the table while preserving the existing id-ordering convention for ties.", + "success_criterion": "Verify Appendix C rows are sorted by section number, with ties broken by req-id." + }, + { + "id": "F7", + "theme": "standalone", + "spec_section": "req-tg-015", + "patch_instruction": "Refine 'native top-level placement' to 'in its native top-level placement (top-level TOML key) in the generated Codex agent file' for readers unfamiliar with config.toml schema.", + "success_criterion": "grep req-tg-015 for 'top-level TOML key'; expect exactly one match." + }, + { + "id": "F8", + "theme": "standalone", + "spec_section": "Appendix D, row 0.1.43", + "patch_instruction": "Align Appendix D wording from 'opt-in conformance capability' to 'consumers providing the accepted Codex-native agent conversion capability' to match the normative clause phrasing.", + "success_criterion": "grep Appendix D 0.1.43 row for 'accepted Codex-native agent conversion capability'; expect exactly one match." + } + ], + "defer_v0_2": [ + { + "id": "F9", + "theme": "standalone", + "spec_section": "openapm-v0.1.requirements.yml schema", + "patch_instruction": "Add an optional 'applicability' key to the requirements manifest schema so conformance tooling can filter conditional requirements structurally without NLP over the 'notes' field. This is an additive schema change affecting all req-tg-* entries.", + "success_criterion": "requirements manifest schema includes an 'applicability' key with type string or object; existing entries validate with the key absent.", + "reserved_slot_anchor": "Section 9.2 (breaking-vs-non-breaking-change-definition) -- additive optional key is non-breaking per 9.2 criteria, but schema-wide manifest changes belong in a coordinated v0.2 revision cycle." + } + ], + "reject": [ + { + "finding_id": "tag-rec-r1-1", + "rationale": "The observation that a target-specific MUST lives inside a target-agnostic section heading is architecturally accurate but the finding itself acknowledges 'the editorial note scope disclaimer is sufficient' for this fold. Refactoring section topology is out of scope for a single-requirement editorial-patch fold; the existing editorial note provides adequate defensive fencing. No action needed in any release track." + } + ], + "ship_decision": "fold_and_ship", + "ship_prose": "All four panels converge on ship_with_followups at a unanimous shocked_meter of 8. Zero blocking findings. The fold-now list (F1-F4) is bounded to four surgical prose additions within the req-tg-015 block and Appendix D -- all codify behavior the reference CLI already implements (printable_ascii_text sanitization, _MAX_DROPPED_FIELDS_SHOWN >= 1, string-only preservation). The drafter can apply these in one mechanical pass. Statement-count arithmetic must be reconciled after F2 and F4 land (each adds one MUST); the linter should re-verify all four enumeration surfaces.\n\nThe defer-v0.1.1 queue (F5-F8) collects SHOULD-level guidance, cosmetic re-sorting, and minor terminology alignment that improve polish but carry no normative risk if deferred. The single defer-v0.2 item (F9: manifest applicability key) is an architectural schema change that would affect every requirement entry and belongs in a coordinated revision cycle, not a single-requirement fold. Recommend fold_and_ship after the drafter lands F1-F4 and the linter confirms count consistency.", + "linter_handoff_notes": "After F1-F4 land: (1) re-grep MUST count -- expect 123 MUST if both F2 and F4 add one MUST each (baseline 121 + req-tg-015 original 1 + F2 + F4 = 123 MUST, total 128 statements with 5 SHOULD); reconcile Section 1.3 opening paragraph, Appendix C footer count, and Appendix D 0.1.43 row. (2) Verify req-tg-015 anchor uniqueness preserved. (3) The orchestrator's prior Wave-5 run recorded checks 1-10 clean and check 11 (no .py files modified, advisory) as a pre-existing advisory note from the broader PR's prior separately-reviewed code work unit -- not a new deviation from this spec-only fold. Re-run the 11-check linter after folds land to confirm no regressions." +} \ No newline at end of file diff --git a/tests/unit/test_apm_spec_guardian_synthesizer_schema.py b/tests/unit/test_apm_spec_guardian_synthesizer_schema.py new file mode 100644 index 0000000000..982f53ed43 --- /dev/null +++ b/tests/unit/test_apm_spec_guardian_synthesizer_schema.py @@ -0,0 +1,312 @@ +"""Regression tests for the apm-spec-guardian synthesizer-return schema. + +Background +---------- +``.apm/skills/apm-spec-guardian/assets/synthesizer-return-schema.json`` is +the canonical (hand-authored) Draft 7 shape the ``spec-editor-synthesizer`` +persona MUST return when invoked by the apm-spec-guardian skill. The +generated mirror at +``.agents/skills/apm-spec-guardian/assets/synthesizer-return-schema.json`` +is produced by ``apm install`` and must stay byte-identical to the +canonical copy. + +The original schema composed ``defer_v0_2.items`` via:: + + "allOf": [{"$ref": "#/definitions/fold_item"}, {"required": [...]}] + +Each ``allOf`` branch validates independently under Draft 7 (no +``unevaluatedProperties``, which is 2019-09+ only), so ``fold_item``'s own +``additionalProperties: false`` (closed over its own 5 properties) rejected +the sibling branch's ``reserved_slot_anchor`` key. This made every +genuinely valid ``defer_v0_2`` entry -- including the real PR #3150 +synthesis return used as a fixture here -- fail schema validation. + +The fix inlines a new definition, ``fold_item_with_reserved_slot``, used +directly (no ``allOf``) by ``defer_v0_2.items``. ``fold_item`` itself +(used bare by ``fold_now`` / ``defer_v0_1_1``) is unchanged. + +Fixture provenance +------------------- +``tests/fixtures/spec-guardian/pr3150-synthesizer-return-original.json`` is +an unedited, verbatim copy of the genuine ``spec-editor-synthesizer`` +return recorded for PR #3150 / issue #3126 (round 1, 2026-10-03, +coordinator session evidence +``spec3150-runtime-evidence/spec-editor-synthesizer-parsed.json``). Its +single ``defer_v0_2`` entry (id ``F9``) cites +``reserved_slot_anchor: "Section 9.2 ..."`` -- Section 9.2 is the +breaking-vs-non-breaking change-classification policy, not an actual +reserved-for-v0.2 slot (contrast the genuinely reserved anchors in +``docs/src/content/docs/specs/openapm-v0.1.md``: Section 4.8 workspaces, +Section 7.9 version withdrawal, Section 10.12 publisher attestations, +Appendix B registry HTTP API). This fixture file intentionally does not +embed any provenance text; it is parsed JSON only. +""" + +from __future__ import annotations + +import copy +import json +from pathlib import Path + +import jsonschema +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +CANONICAL_SCHEMA_PATH = ( + REPO_ROOT / ".apm/skills/apm-spec-guardian/assets/synthesizer-return-schema.json" +) +GENERATED_MIRROR_PATH = ( + REPO_ROOT / ".agents/skills/apm-spec-guardian/assets/synthesizer-return-schema.json" +) +ORIGINAL_RETURN_FIXTURE = ( + REPO_ROOT / "tests/fixtures/spec-guardian/pr3150-synthesizer-return-original.json" +) + + +def _load_canonical_schema() -> dict: + with CANONICAL_SCHEMA_PATH.open(encoding="utf-8") as handle: + return json.load(handle) + + +def _load_original_return() -> dict: + with ORIGINAL_RETURN_FIXTURE.open(encoding="utf-8") as handle: + return json.load(handle) + + +def _reconstruct_pre_fix_schema(fixed_schema: dict) -> dict: + """Build a disposable in-memory copy reproducing the ORIGINAL allOf bug. + + Never touches the committed schema file. Used only to prove the + before/after behavior of the fix and that the negative guards remain + load-bearing. Starts from a deep copy of the fixed schema and replaces + ``defer_v0_2.items`` with the original closed-allOf composition plus a + bare ``reserved_slot_anchor`` requirement, mirroring exactly what was + on disk before this fix. + """ + broken = copy.deepcopy(fixed_schema) + broken["properties"]["defer_v0_2"]["items"] = { + "allOf": [ + {"$ref": "#/definitions/fold_item"}, + { + "type": "object", + "required": ["reserved_slot_anchor"], + "properties": { + "reserved_slot_anchor": { + "type": "string", + "description": ( + "Section / anchor in the current artifact that " + "already reserves space for this work." + ), + } + }, + }, + ] + } + return broken + + +def _minimal_valid_report(defer_v0_2_entry: dict | None = None) -> dict: + """A synthetic, minimally-valid full report for isolated-field tests.""" + fold_item = { + "id": "F1", + "theme": "standalone", + "spec_section": "req-tg-000", + "patch_instruction": "example instruction", + "success_criterion": "example criterion", + } + report = { + "round": 1, + "shocked_meter_avg": 8.0, + "convergence_table": [ + { + "panel": panel, + "verdict": "ship_with_followups", + "shocked_meter": 8, + "new_blockers": 0, + "new_recommended": 0, + "new_nits": 0, + } + for panel in ( + "spec-swagger-editor", + "spec-oci-editor", + "spec-pkgmgr-editor", + "spec-tag-architect", + ) + ], + "convergent_themes": [], + "fold_now": [fold_item], + "defer_v0_1_1": [fold_item], + "defer_v0_2": [defer_v0_2_entry] if defer_v0_2_entry is not None else [], + "reject": [], + "ship_decision": "fold_and_ship", + "ship_prose": "example ship prose", + "linter_handoff_notes": "", + } + return report + + +def _valid_defer_v0_2_entry() -> dict: + return { + "id": "F9", + "theme": "standalone", + "spec_section": "sec.4.8", + "patch_instruction": "example instruction", + "success_criterion": "example criterion", + "reserved_slot_anchor": "sec.4.8 workspaces reserved", + } + + +def test_schema_document_is_valid_draft7() -> None: + schema = _load_canonical_schema() + assert schema["$schema"] == "http://json-schema.org/draft-07/schema#" + jsonschema.Draft7Validator.check_schema(schema) + + +def test_valid_full_report_and_original_return_fail_broken_schema_pass_fixed() -> None: + fixed_schema = _load_canonical_schema() + broken_schema = _reconstruct_pre_fix_schema(fixed_schema) + + synthetic_report = _minimal_valid_report(_valid_defer_v0_2_entry()) + original_return = _load_original_return() + + for instance in (synthetic_report, original_return): + with pytest.raises(jsonschema.ValidationError): + jsonschema.validate(instance, broken_schema) + jsonschema.validate(instance, fixed_schema) + + +def test_defer_v0_2_missing_reserved_slot_anchor_rejected() -> None: + fixed_schema = _load_canonical_schema() + entry = _valid_defer_v0_2_entry() + del entry["reserved_slot_anchor"] + report = _minimal_valid_report(entry) + + with pytest.raises(jsonschema.ValidationError): + jsonschema.validate(report, fixed_schema) + + # Load-bearing proof: a disposable in-memory copy with the `required` + # constraint dropped from fold_item_with_reserved_slot would WRONGLY + # accept this same instance -- confirming the real schema's `required` + # clause is what makes the rejection above happen, not an unrelated + # failure. + weakened = copy.deepcopy(fixed_schema) + weakened["definitions"]["fold_item_with_reserved_slot"]["required"] = [ + "id", + "theme", + "spec_section", + "patch_instruction", + "success_criterion", + ] + jsonschema.validate(report, weakened) + + +def test_defer_v0_2_invalid_reserved_slot_anchor_type_rejected() -> None: + fixed_schema = _load_canonical_schema() + entry = _valid_defer_v0_2_entry() + entry["reserved_slot_anchor"] = 42 + report = _minimal_valid_report(entry) + + with pytest.raises(jsonschema.ValidationError): + jsonschema.validate(report, fixed_schema) + + +@pytest.mark.parametrize("section", ["fold_now", "defer_v0_1_1", "defer_v0_2"]) +def test_unknown_property_rejected_in_each_section(section: str) -> None: + fixed_schema = _load_canonical_schema() + entry = ( + _valid_defer_v0_2_entry() + if section == "defer_v0_2" + else { + "id": "F1", + "theme": "standalone", + "spec_section": "req-tg-000", + "patch_instruction": "example instruction", + "success_criterion": "example criterion", + } + ) + entry["unexpected_key"] = "should not be allowed" + report = _minimal_valid_report() + report[section] = [entry] + + with pytest.raises(jsonschema.ValidationError): + jsonschema.validate(report, fixed_schema) + + if section == "defer_v0_2": + # Load-bearing proof for the closure guard specifically: flipping + # additionalProperties to true on the disposable in-memory copy + # would wrongly accept the same instance. + weakened = copy.deepcopy(fixed_schema) + weakened["definitions"]["fold_item_with_reserved_slot"]["additionalProperties"] = True + jsonschema.validate(report, weakened) + + +def test_unknown_property_rejected_at_top_level() -> None: + fixed_schema = _load_canonical_schema() + report = _minimal_valid_report() + report["unexpected_top_level_key"] = "nope" + + with pytest.raises(jsonschema.ValidationError): + jsonschema.validate(report, fixed_schema) + + +def test_malformed_id_pattern_and_shocked_meter_type_rejected() -> None: + fixed_schema = _load_canonical_schema() + + bad_id_entry = _valid_defer_v0_2_entry() + bad_id_entry["id"] = "not-an-f-id" + bad_id_report = _minimal_valid_report(bad_id_entry) + with pytest.raises(jsonschema.ValidationError): + jsonschema.validate(bad_id_report, fixed_schema) + + bad_meter_report = _minimal_valid_report(_valid_defer_v0_2_entry()) + bad_meter_report["shocked_meter_avg"] = "eight" + with pytest.raises(jsonschema.ValidationError): + jsonschema.validate(bad_meter_report, fixed_schema) + + +def test_fold_now_and_defer_v0_1_1_unaffected_by_fix() -> None: + """Bare fold_item entries (no reserved_slot_anchor) still validate.""" + fixed_schema = _load_canonical_schema() + report = _minimal_valid_report() + jsonschema.validate(report, fixed_schema) + + +def test_shared_fold_item_properties_structurally_identical() -> None: + """fold_item and fold_item_with_reserved_slot must not silently drift. + + The five base properties are intentionally duplicated (not $ref-shared) + to keep fold_item_with_reserved_slot a single closed object expressible + in Draft 7. This test is the explicit parity guard the duplication + requires. + """ + schema = _load_canonical_schema() + base = schema["definitions"]["fold_item"] + extended = schema["definitions"]["fold_item_with_reserved_slot"] + + shared_keys = ["id", "theme", "spec_section", "patch_instruction", "success_criterion"] + assert set(base["required"]) == set(shared_keys) + assert set(shared_keys).issubset(set(extended["required"])) + + for key in shared_keys: + assert base["properties"][key] == extended["properties"][key], ( + f"fold_item.properties.{key} drifted from fold_item_with_reserved_slot.properties.{key}" + ) + + +def test_canonical_and_generated_copy_are_byte_identical() -> None: + """The .agents/ mirror must match the canonical .apm/ source exactly. + + This fails until `apm install` has regenerated the mirror. It is a + deliberate, reported gate -- not something this test silently skips. + """ + if not GENERATED_MIRROR_PATH.exists(): + pytest.fail( + f"generated mirror missing at {GENERATED_MIRROR_PATH}; " + "run `apm install` to regenerate it before this test can pass" + ) + canonical_bytes = CANONICAL_SCHEMA_PATH.read_bytes() + mirror_bytes = GENERATED_MIRROR_PATH.read_bytes() + assert canonical_bytes == mirror_bytes, ( + "canonical schema and generated .agents/ mirror have diverged; " + "run `apm install` to regenerate the mirror" + ) From 4b7e4118bb4250691fdcdfa6db44ce24dbdffc3d Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 5 Oct 2026 11:34:37 +0200 Subject: [PATCH 10/13] chore(apm-spec-guardian): regenerate deployed schema mirror and lockfile hashes Run uv run --frozen apm install to regenerate the deployed .agents/skills/apm-spec-guardian/assets/synthesizer-return-schema.json mirror from the canonical .apm/ source after the Draft7 schema fix, and update the two apm.lock.yaml fields that track the hash of that file (deployments[].content_hash and local_deployed_file_hashes entry). uv.lock is untouched. apm audit --ci reports no drift; all 10 policy checks pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../assets/synthesizer-return-schema.json | 33 ++++++++++--------- apm.lock.yaml | 4 +-- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/.agents/skills/apm-spec-guardian/assets/synthesizer-return-schema.json b/.agents/skills/apm-spec-guardian/assets/synthesizer-return-schema.json index 7c55b591bc..a72d177b88 100644 --- a/.agents/skills/apm-spec-guardian/assets/synthesizer-return-schema.json +++ b/.agents/skills/apm-spec-guardian/assets/synthesizer-return-schema.json @@ -93,21 +93,7 @@ "defer_v0_2": { "type": "array", "description": "Architectural work bound to a reserved slot in a future major. Each entry MUST cite the existing reserved-slot section anchor in the artifact (e.g. 'sec.4.8 workspaces reserved', 'sec.10.12 publisher attestations reserved').", - "items": { - "allOf": [ - { "$ref": "#/definitions/fold_item" }, - { - "type": "object", - "required": ["reserved_slot_anchor"], - "properties": { - "reserved_slot_anchor": { - "type": "string", - "description": "Section / anchor in the current artifact that already reserves space for this work." - } - } - } - ] - } + "items": { "$ref": "#/definitions/fold_item_with_reserved_slot" } }, "reject": { "type": "array", @@ -148,6 +134,23 @@ "patch_instruction": { "type": "string", "description": "Concrete mechanical instruction the drafter applies. ASCII only. MUST NOT add any forbidden vendor / foundation token to the artifact." }, "success_criterion": { "type": "string", "description": "Verifiable post-fold check (grep, schema validation, or anchor count)." } } + }, + "fold_item_with_reserved_slot": { + "type": "object", + "description": "Same shape as fold_item plus a required reserved_slot_anchor. Inlined (not composed via allOf over fold_item) because Draft 7 has no unevaluatedProperties: an allOf branch's own additionalProperties:false would otherwise reject the sibling branch's reserved_slot_anchor key. The five base properties are intentionally duplicated here, not $ref-shared, to keep this a single closed object; test_shared_fold_item_properties_structurally_identical guards against the two copies drifting apart.", + "required": ["id", "theme", "spec_section", "patch_instruction", "success_criterion", "reserved_slot_anchor"], + "additionalProperties": false, + "properties": { + "id": { "type": "string", "pattern": "^F[1-9][0-9]*$" }, + "theme": { "type": "string", "description": "Convergent-theme id this fold addresses (e.g. T1, T2), or 'standalone' if not part of a clustered theme." }, + "spec_section": { "type": "string", "description": "Section / appendix / req-XXX anchor where the patch lands." }, + "patch_instruction": { "type": "string", "description": "Concrete mechanical instruction the drafter applies. ASCII only. MUST NOT add any forbidden vendor / foundation token to the artifact." }, + "success_criterion": { "type": "string", "description": "Verifiable post-fold check (grep, schema validation, or anchor count)." }, + "reserved_slot_anchor": { + "type": "string", + "description": "Section / anchor in the current artifact that already reserves space for this work." + } + } } } } diff --git a/apm.lock.yaml b/apm.lock.yaml index c255739d6d..1cf1208087 100644 --- a/apm.lock.yaml +++ b/apm.lock.yaml @@ -1590,7 +1590,7 @@ deployments: owners: - . active_owner: . - content_hash: sha256:e6e2771f2fe8578e72d12e51edcc3817717f8a0bbef66d8a8a5c556f0dce06d7 + content_hash: sha256:2115c15be67270f2b969b7f49e88d9686382d4aed10fed21ae47811c36981c98 - kind: project-relative target: legacy value: .agents/skills/apm-strategy @@ -3104,7 +3104,7 @@ local_deployed_file_hashes: .agents/skills/apm-spec-guardian/assets/comment-template.md: sha256:8f5703aa98d1036d2418ffa45eac9d0270504dac33f3e9b1892884a9d48da1a6 .agents/skills/apm-spec-guardian/assets/linter-checklist.md: sha256:b3b60af18f4b37e9f68815aaf8001617b81cc24fb8fe0a1f19074db93efa6b3b .agents/skills/apm-spec-guardian/assets/panelist-return-schema.json: sha256:bb66b270394c386665c6b93d83befe05ce9f8206abf8d656079c29d65729fefd - .agents/skills/apm-spec-guardian/assets/synthesizer-return-schema.json: sha256:e6e2771f2fe8578e72d12e51edcc3817717f8a0bbef66d8a8a5c556f0dce06d7 + .agents/skills/apm-spec-guardian/assets/synthesizer-return-schema.json: sha256:2115c15be67270f2b969b7f49e88d9686382d4aed10fed21ae47811c36981c98 .agents/skills/apm-strategy/SKILL.md: sha256:6c7fbc821fef930ab76764c7b33d55bd181f31cdd406cff5ce7cf14d1071c2ef .agents/skills/auth/SKILL.md: sha256:bf1039cb63dbb968d50dbf4a9e4ceec9a4c30e40e8dd666d404f1c39eda55bc0 .agents/skills/cli-logging-ux/SKILL.md: sha256:b04d5282a9f21b33c75ac98c41c7eab7e562eafea8c89bbb5732d9956958fe7c From fe455ab577c147667e157fcfe86034b875dcc870 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 5 Oct 2026 15:02:59 +0200 Subject: [PATCH 11/13] Fold full-panel review nits: DRY field constants, tighter typing, boundary tests - Extract _CODEX_MODEL_FIELDS / _CODEX_KNOWN_FIELDS class constants to remove field-vocabulary duplication between model_fields and the dropped_field_names known-field exclusion set. - Tighten _display_dropped_field_key's parameter type from object to str | int | float | bool | None, matching the actual accepted domain. - Add off-by-one boundary tests for the dropped-field count cap (_MAX_DROPPED_FIELDS_SHOWN) and per-key length cap (_MAX_DROPPED_FIELD_KEY_LEN), verified by mutation-break (removing the guard fails the new boundary test). These fold the 3 nits (zero blocking/recommended findings) surfaced by the full review panel (python-architect, test-coverage-expert, doc-writer, apm-ceo) for #3150. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/src/content/docs/specs/openapm-v0.1.md | 5 +- src/apm_cli/integration/agent_integrator.py | 15 ++- .../unit/integration/test_agent_integrator.py | 102 ++++++++++++++++++ 3 files changed, 115 insertions(+), 7 deletions(-) diff --git a/docs/src/content/docs/specs/openapm-v0.1.md b/docs/src/content/docs/specs/openapm-v0.1.md index fd262bd4d1..b68f1589c9 100644 --- a/docs/src/content/docs/specs/openapm-v0.1.md +++ b/docs/src/content/docs/specs/openapm-v0.1.md @@ -2689,7 +2689,8 @@ intersection are not subject to this gate. **[req-tg-015]** A consumer implementation providing the accepted Codex-native agent conversion capability MUST preserve a source-declared `model` or `model_reasoning_effort` string value in -its native top-level placement in the generated Codex agent file, and +its native top-level placement (top-level TOML key) in the generated +Codex agent file, and MUST leave either field absent from the generated file when it is absent from the source. This clause does not require or imply preservation of any other `config.toml`-native key, and does not @@ -4166,7 +4167,7 @@ renumbering of conformance classes. | 0.1.40 | 2026-09-07 | Spec-citation fold for dependency-policy identity casing in PR #2706. Added [req-pl-018] (Section 6.3.1, governance MUST) and extended [req-rs-016] clause (3): dependency allow, deny, and exact require operands use the documented per-host repository case rule, while registry-sourced repository coordinates are case-insensitive regardless of host; case normalization is ASCII-only, is bounded identically on both operands, stops at recursive-glob ambiguity, and does not cross virtual-path, ref, registry-name, MCP-name, unmanaged-path, or case-sensitive host/source boundaries; deny precedence is unchanged. Defined the policy glob grammar, documented byte-exact Section 6.4 merge behavior, and added the threat mapping. Classified this as a non-breaking correction of previously unspecified evaluation behavior under Section 9.2: existing lowercase workarounds remain matching; on registry sources and hosts documented as case-insensitive, case-variant allow entries can newly match, deny entries can newly enforce, and exact require entries can newly be satisfied, so those policies should be re-audited. Sections 1.3, 6.3.1, 6.3.5, 6.4, 6.5, 6.9, 7.2, 9.2, 10.8, 10.11, 11.2, and 11.3.4, Appendix C, and conformance coverage updated. Statement count: 121 -> 122 (117 MUST, 5 SHOULD). | | 0.1.41 | 2026-09-09 | Alias containment and lock-replay contract for PR #2901. Added [req-mf-025] (Section 4.3.2, consumer MUST), the optional lock-entry `alias` field, and conformance coverage. Under Section 9.2 this is an additive optional field and a defensive definition of previously unspecified alias behavior, not behavior-neutral errata: unsafe or reserved aliases can newly fail; valid dotted aliases remain accepted; surrounding whitespace is canonicalized; recorded aliases determine replay placement; absent aliases retain the unaliased layout. Source identity and permitted local source paths are unchanged. Older readers preserving the unknown field do not thereby implement placement support. Selects distinct 0.1.41 schema publication identities without changing published v0.1 URLs or bytes; Section 9.3 remains pending (see Appendix A). Sections 1.3, 4.9, 5.2, 10.7, 10.11, 11.3.2, and Appendix C updated. Statement count: 122 -> 123 (118 MUST, 5 SHOULD). | | 0.1.42 (proposed) | 2026-09-30 | Optional deployed-prompt audit capability for PR #2962. Added conditional governance [req-pl-019] and [req-pl-020] in Section 6.8.1, both enumerations, Appendix C, requirements manifest and behavioral conformance coverage. Under Section 9.2 this is a new opt-in conformance capability, not behavior-neutral errata or a reinterpretation of an existing obligation: implementations not claiming it acquire no new required feature. APM claims it; newly discovered prompt findings and incomplete native coverage can newly fail default and CI audits, command-only content remains non-failing, and protected remediation is refused before writes. No schema, lockfile version, existing mandatory feature, drift-policy or ownership rule changes. Section 9.3 reviewer approvals and public comment period remain pending; this proposal is not evidence of adoption. Statement count: 123 -> 125 (120 MUST, 5 SHOULD). | -| 0.1.43 (proposed) | 2026-10-03 | Codex-native agent model preservation and bounded dropped-metadata diagnostic for PR #3150 (closes #3126). Added [req-tg-015] (Section 8.5.1, consumer MUST): a consumer providing the accepted Codex-native agent conversion capability MUST preserve a source-declared `model` or `model_reasoning_effort` string value in its native top-level placement, MUST leave either field absent when absent from the source, and MUST emit a diagnostic bounded on both dropped-field count and per-field name length (never the value) for any other dropped, non-capability-restriction frontmatter field. Under Section 9.2 this is a new opt-in conformance capability scoped to the Codex-native conversion path, not behavior-neutral errata: it does not require or imply preservation of any other `config.toml`-native key and does not define behavior for any other conversion target. Implementations not providing Codex-native agent conversion acquire no new required feature. No schema, lockfile version, existing mandatory feature, drift-policy or ownership rule changes. Section 9.3 reviewer approvals and public comment period remain pending; this proposal is not evidence of adoption. Section 8.7, Section 11.3.2, and Appendix C updated. Statement count: 125 -> 126 (121 MUST, 5 SHOULD). | +| 0.1.43 (proposed) | 2026-10-03 | Codex-native agent model preservation and bounded dropped-metadata diagnostic for PR #3150 (closes #3126). Added [req-tg-015] (Section 8.5.1, consumer MUST): a consumer providing the accepted Codex-native agent conversion capability MUST preserve a source-declared `model` or `model_reasoning_effort` string value in its native top-level placement, MUST leave either field absent when absent from the source, and MUST emit a diagnostic bounded on both dropped-field count and per-field name length (never the value) for any other dropped, non-capability-restriction frontmatter field. Under Section 9.2 this is a new opt-in conformance capability scoped to consumers providing the accepted Codex-native agent conversion capability, not behavior-neutral errata: it does not require or imply preservation of any other `config.toml`-native key and does not define behavior for any other conversion target. Implementations not providing Codex-native agent conversion acquire no new required feature. No schema, lockfile version, existing mandatory feature, drift-policy or ownership rule changes. Section 9.3 reviewer approvals and public comment period remain pending; this proposal is not evidence of adoption. Section 8.7, Section 11.3.2, and Appendix C updated. Statement count: 125 -> 126 (121 MUST, 5 SHOULD). | Errata (none at publication). diff --git a/src/apm_cli/integration/agent_integrator.py b/src/apm_cli/integration/agent_integrator.py index 2da8e36f40..23c1e9dca9 100644 --- a/src/apm_cli/integration/agent_integrator.py +++ b/src/apm_cli/integration/agent_integrator.py @@ -423,8 +423,15 @@ def _warn_opencode_frontmatter( # bounds the whole diagnostic's total length. _MAX_DROPPED_FIELD_KEY_LEN = 60 + # Native string fields Codex itself recognizes and whose declared value + # takes precedence over any APM default. Single source of truth for both + # the model-settings extraction loop and the dropped-fields exclusion set + # below, so adding a third recognized field only requires one edit here. + _CODEX_MODEL_FIELDS = ("model", "model_reasoning_effort") + _CODEX_KNOWN_FIELDS = frozenset({"name", "description", "tools", *_CODEX_MODEL_FIELDS}) + @staticmethod - def _display_dropped_field_key(field: object) -> str: + def _display_dropped_field_key(field: str | int | float | bool | None) -> str: """Render one dropped frontmatter key name, bounded and ASCII-safe. Sanitizes control/non-ASCII characters first, then truncates the @@ -502,7 +509,7 @@ def _write_codex_agent( name = name[: -len(".agent")] description = "" body = content - model_fields = ("model", "model_reasoning_effort") + model_fields = AgentIntegrator._CODEX_MODEL_FIELDS model_settings: dict[str, str] = {} fm_match = AgentIntegrator._FRONTMATTER_RE.match(content) @@ -534,9 +541,7 @@ def _write_codex_agent( # so hostile frontmatter with many keys does not force # rendering every key just to discard most of them below. dropped_field_names = [ - field - for field in fm - if field not in {"name", "description", "tools", *model_fields} + field for field in fm if field not in AgentIntegrator._CODEX_KNOWN_FIELDS ] if dropped_field_names and diagnostics is not None: shown_names = dropped_field_names[ diff --git a/tests/unit/integration/test_agent_integrator.py b/tests/unit/integration/test_agent_integrator.py index 831978f10c..28918a0786 100644 --- a/tests/unit/integration/test_agent_integrator.py +++ b/tests/unit/integration/test_agent_integrator.py @@ -1372,6 +1372,108 @@ def test_codex_dropped_field_with_oversized_key_name_is_bounded(self) -> None: assert "...(truncated)" in message assert len(message) < 300 + @pytest.mark.parametrize( + ("key_count", "expect_elision"), + [ + pytest.param( + AgentIntegrator._MAX_DROPPED_FIELDS_SHOWN, + False, + id="at-cap-no-elision", + ), + pytest.param( + AgentIntegrator._MAX_DROPPED_FIELDS_SHOWN + 1, + True, + id="one-over-cap-elides-one", + ), + ], + ) + def test_codex_dropped_fields_count_boundary( + self, key_count: int, expect_elision: bool + ) -> None: + """Exercise the exact off-by-one boundary of the count cap. + + The pre-existing coverage only proves bounding works far above the + cap (9 keys vs a cap of 5); this proves the cap's own edge: exactly + `_MAX_DROPPED_FIELDS_SHOWN` keys must name all of them with no + "(and N more)" suffix, while one more key must add exactly that + suffix for exactly one elided key. + """ + source = self.root / "boundary-count.agent.md" + unsupported = {f"field_{i}": "value" for i in range(key_count)} + frontmatter = {"name": "reviewer", **unsupported} + source.write_text( + f"---\n{yaml_to_str(frontmatter)}---\nReview changes.\n", encoding="utf-8" + ) + target = self.root / "reviewer.toml" + diagnostics = DiagnosticCollector() + + AgentIntegrator._write_codex_agent( + source, target, diagnostics=diagnostics, package_name="test-pkg" + ) + + warnings = diagnostics.by_category().get(CATEGORY_AGENT_LOSSY_COMPILATION, []) + assert len(warnings) == 1 + message = warnings[0].message + shown_keys = list(unsupported)[: AgentIntegrator._MAX_DROPPED_FIELDS_SHOWN] + elided_keys = list(unsupported)[AgentIntegrator._MAX_DROPPED_FIELDS_SHOWN :] + for key in shown_keys: + assert f"'{key}'" in message + if expect_elision: + assert "(and 1 more)" in message + for key in elided_keys: + assert f"'{key}'" not in message + else: + assert "(and" not in message + assert not elided_keys + + @pytest.mark.parametrize( + ("key_len", "expect_truncation"), + [ + pytest.param( + AgentIntegrator._MAX_DROPPED_FIELD_KEY_LEN, + False, + id="at-cap-no-truncation", + ), + pytest.param( + AgentIntegrator._MAX_DROPPED_FIELD_KEY_LEN + 1, + True, + id="one-over-cap-truncates", + ), + ], + ) + def test_codex_dropped_field_key_length_boundary( + self, key_len: int, expect_truncation: bool + ) -> None: + """Exercise the exact off-by-one boundary of the per-key length cap. + + The pre-existing coverage only proves truncation works far above + the cap (20000 chars vs a cap of 60); this proves the cap's own + edge: a key exactly `_MAX_DROPPED_FIELD_KEY_LEN` chars long must + display whole, while one char longer must be truncated. + """ + source = self.root / "boundary-length.agent.md" + key = "k" * key_len + frontmatter = {"name": "reviewer", key: "value"} + source.write_text( + f"---\n{yaml_to_str(frontmatter)}---\nReview changes.\n", encoding="utf-8" + ) + target = self.root / "reviewer.toml" + diagnostics = DiagnosticCollector() + + AgentIntegrator._write_codex_agent( + source, target, diagnostics=diagnostics, package_name="test-pkg" + ) + + warnings = diagnostics.by_category().get(CATEGORY_AGENT_LOSSY_COMPILATION, []) + assert len(warnings) == 1 + message = warnings[0].message + if expect_truncation: + assert key not in message + assert "...(truncated)" in message + else: + assert f"'{key}'" in message + assert "...(truncated)" not in message + @pytest.mark.parametrize("target_name", _verbatim_copy_agent_targets()) def test_codex_metadata_change_preserves_other_targets(self, target_name: str) -> None: """Codex filtering must not leak into verbatim target deployment.""" From 24f18c7d3f7be1269381fe15a936521fb1866c65 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 5 Oct 2026 15:10:27 +0200 Subject: [PATCH 12/13] Correct _display_dropped_field_key type: revert to object The prior fold narrowed this parameter to str | int | float | bool | None, but that excludes real values the frontmatter YAML loader (load_yaml_str, a bounded PyYAML SafeLoader) actually produces for mapping keys: datetime.date (bare 2026-10-05: keys) and bytes (!!binary keys). Verified empirically: load_yaml_str('2026-10-05: ignored\n? !!binary aGVsbG8=\n: ignored\n') -> {datetime.date(2026, 10, 5): 'ignored', b'hello': 'ignored'} Restoring object (with a docstring note) preserves existing behavior for all legal YAML frontmatter keys instead of silently under-typing a path that str(field) already handles correctly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/integration/agent_integrator.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/apm_cli/integration/agent_integrator.py b/src/apm_cli/integration/agent_integrator.py index 23c1e9dca9..50922eb657 100644 --- a/src/apm_cli/integration/agent_integrator.py +++ b/src/apm_cli/integration/agent_integrator.py @@ -431,9 +431,16 @@ def _warn_opencode_frontmatter( _CODEX_KNOWN_FIELDS = frozenset({"name", "description", "tools", *_CODEX_MODEL_FIELDS}) @staticmethod - def _display_dropped_field_key(field: str | int | float | bool | None) -> str: + def _display_dropped_field_key(field: object) -> str: """Render one dropped frontmatter key name, bounded and ASCII-safe. + ``field`` stays ``object`` (not a narrower union) because the YAML + frontmatter loader (``load_yaml_str``) accepts the full YAML 1.1 key + domain, including ``datetime.date`` (bare ``2026-10-05:`` keys) and + ``bytes`` (``!!binary`` keys) -- neither representable by a small + str/int/float/bool/None union. ``str(field)`` below covers all of + them uniformly. + Sanitizes control/non-ASCII characters first, then truncates the *displayed* key (never the full value) so a single oversized key name cannot blow up the diagnostic line. From d298a419a1acffde8104802432e0936c71c1a160 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Tue, 6 Oct 2026 11:12:32 +0200 Subject: [PATCH 13/13] fix(codex): fold diagnostic and conformance review findings Address complete-input Oct 6 panel follow-ups for #3126: preserve newline settings under parsed TOML, use singular metadata warnings, and clarify the bounded proposed spec contract without invented companion provenance or broader target semantics. Correct unreleased wording and align authoring guidance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- .../specs/manifests/openapm-v0.1.requirements.yml | 2 +- docs/src/content/docs/specs/openapm-v0.1.md | 15 ++++++++++++--- .../.apm/skills/apm-usage/package-authoring.md | 4 ++-- src/apm_cli/integration/agent_integrator.py | 14 +++++++++----- .../test_codex_agent_tool_scope_contract.py | 2 +- tests/spec_conformance/test_manifest_reqs.py | 5 ++++- tests/unit/integration/test_agent_integrator.py | 11 +++++++++++ 8 files changed, 41 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e27f1a602e..9b87fefe93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- `docs/src/content/docs/specs/openapm-v0.1.md` adds `req-tg-015`, documenting the already-shipped Codex-native `model`/`model_reasoning_effort` preservation and bounded dropped-metadata diagnostic as a normative conformance requirement. (#3150) +- `docs/src/content/docs/specs/openapm-v0.1.md` adds proposed `req-tg-015` for Codex-native `model`/`model_reasoning_effort` preservation and bounded dropped-metadata diagnostics. (#3150) ### Fixed diff --git a/docs/public/specs/manifests/openapm-v0.1.requirements.yml b/docs/public/specs/manifests/openapm-v0.1.requirements.yml index 3f8b072aaf..080f7b463e 100644 --- a/docs/public/specs/manifests/openapm-v0.1.requirements.yml +++ b/docs/public/specs/manifests/openapm-v0.1.requirements.yml @@ -464,7 +464,7 @@ requirements: keyword: MUST section: "8.5.1" conformance_class: consumer - notes: "a consumer providing the accepted Codex-native agent conversion capability preserves a source-declared model or model_reasoning_effort string value in its native top-level placement, leaves either field absent when absent from the source, still diagnoses a non-string source value for either field, and emits a diagnostic bounded on dropped-field count (at least one shown when any are dropped), per-field name length, and ASCII-sanitized field-name display (never the value) for any other dropped, non-capability-restriction frontmatter field" + notes: "a conforming consumer providing the Codex-native agent conversion capability preserves a source-declared model or model_reasoning_effort string value in its native top-level placement, leaves either field absent when absent from the source, diagnoses a non-string source value by field name without including that value, and emits a diagnostic bounded on dropped-field count (at least one shown when any are dropped), per-field name length, and ASCII-sanitized field-name display (never the value) for any other dropped, non-capability-restriction frontmatter field" - id: req-pr-006 keyword: MUST section: "8.1" diff --git a/docs/src/content/docs/specs/openapm-v0.1.md b/docs/src/content/docs/specs/openapm-v0.1.md index b68f1589c9..d8c2ecd715 100644 --- a/docs/src/content/docs/specs/openapm-v0.1.md +++ b/docs/src/content/docs/specs/openapm-v0.1.md @@ -2686,7 +2686,7 @@ intersection are not subject to this gate. > companion. -**[req-tg-015]** A consumer implementation providing the accepted +**[req-tg-015]** A conforming **consumer** implementation providing the Codex-native agent conversion capability MUST preserve a source-declared `model` or `model_reasoning_effort` string value in its native top-level placement (top-level TOML key) in the generated @@ -2697,7 +2697,8 @@ preservation of any other `config.toml`-native key, and does not define behavior for any other conversion target. A source-declared `model` or `model_reasoning_effort` value that is not a YAML string scalar is not subject to preservation under this clause, and the -consumer MUST still emit a diagnostic for it. +consumer MUST still emit a diagnostic for it. That diagnostic MUST +identify the field name but MUST NOT include the rejected value. When that same consumer, converting to the Codex-native format, drops a declared frontmatter field that is neither a @@ -2721,6 +2722,9 @@ is unaffected and continues to apply on its own terms. > consumers that already provide Codex-native agent conversion; it does > not standardize dropped-metadata handling for any other target, and > does not introduce a new vendor-namespaced metadata system. +> The two fields are native Codex agent settings; their string values +> are opaque to this specification. This clause does not prescribe +> TOML field ordering or a cross-target diagnostic-output contract. #### 8.5.2 Post-install compilation guidance @@ -3376,6 +3380,11 @@ every stored hash, foreclosing algorithm-ambiguity attacks. | 20| Source-only or symlinked package content materialization | [req-sc-015](#req-sc-015) | Consumer-default | | 21| Native plugin namespace collision or ownership-ledger loss | [req-tg-013](#req-tg-013) | Consumer-default | +For Codex conversion metadata diagnostics, [req-tg-015](#req-tg-015) +separately constrains field-name display and excludes dropped values. +This diagnostic-safety contract is distinct from row 16's +capability-scope preservation. + ### 10.12 Publisher provenance and attestations (reserved for v0.2) Publisher provenance (cryptographic attestations binding a @@ -4167,7 +4176,7 @@ renumbering of conformance classes. | 0.1.40 | 2026-09-07 | Spec-citation fold for dependency-policy identity casing in PR #2706. Added [req-pl-018] (Section 6.3.1, governance MUST) and extended [req-rs-016] clause (3): dependency allow, deny, and exact require operands use the documented per-host repository case rule, while registry-sourced repository coordinates are case-insensitive regardless of host; case normalization is ASCII-only, is bounded identically on both operands, stops at recursive-glob ambiguity, and does not cross virtual-path, ref, registry-name, MCP-name, unmanaged-path, or case-sensitive host/source boundaries; deny precedence is unchanged. Defined the policy glob grammar, documented byte-exact Section 6.4 merge behavior, and added the threat mapping. Classified this as a non-breaking correction of previously unspecified evaluation behavior under Section 9.2: existing lowercase workarounds remain matching; on registry sources and hosts documented as case-insensitive, case-variant allow entries can newly match, deny entries can newly enforce, and exact require entries can newly be satisfied, so those policies should be re-audited. Sections 1.3, 6.3.1, 6.3.5, 6.4, 6.5, 6.9, 7.2, 9.2, 10.8, 10.11, 11.2, and 11.3.4, Appendix C, and conformance coverage updated. Statement count: 121 -> 122 (117 MUST, 5 SHOULD). | | 0.1.41 | 2026-09-09 | Alias containment and lock-replay contract for PR #2901. Added [req-mf-025] (Section 4.3.2, consumer MUST), the optional lock-entry `alias` field, and conformance coverage. Under Section 9.2 this is an additive optional field and a defensive definition of previously unspecified alias behavior, not behavior-neutral errata: unsafe or reserved aliases can newly fail; valid dotted aliases remain accepted; surrounding whitespace is canonicalized; recorded aliases determine replay placement; absent aliases retain the unaliased layout. Source identity and permitted local source paths are unchanged. Older readers preserving the unknown field do not thereby implement placement support. Selects distinct 0.1.41 schema publication identities without changing published v0.1 URLs or bytes; Section 9.3 remains pending (see Appendix A). Sections 1.3, 4.9, 5.2, 10.7, 10.11, 11.3.2, and Appendix C updated. Statement count: 122 -> 123 (118 MUST, 5 SHOULD). | | 0.1.42 (proposed) | 2026-09-30 | Optional deployed-prompt audit capability for PR #2962. Added conditional governance [req-pl-019] and [req-pl-020] in Section 6.8.1, both enumerations, Appendix C, requirements manifest and behavioral conformance coverage. Under Section 9.2 this is a new opt-in conformance capability, not behavior-neutral errata or a reinterpretation of an existing obligation: implementations not claiming it acquire no new required feature. APM claims it; newly discovered prompt findings and incomplete native coverage can newly fail default and CI audits, command-only content remains non-failing, and protected remediation is refused before writes. No schema, lockfile version, existing mandatory feature, drift-policy or ownership rule changes. Section 9.3 reviewer approvals and public comment period remain pending; this proposal is not evidence of adoption. Statement count: 123 -> 125 (120 MUST, 5 SHOULD). | -| 0.1.43 (proposed) | 2026-10-03 | Codex-native agent model preservation and bounded dropped-metadata diagnostic for PR #3150 (closes #3126). Added [req-tg-015] (Section 8.5.1, consumer MUST): a consumer providing the accepted Codex-native agent conversion capability MUST preserve a source-declared `model` or `model_reasoning_effort` string value in its native top-level placement, MUST leave either field absent when absent from the source, and MUST emit a diagnostic bounded on both dropped-field count and per-field name length (never the value) for any other dropped, non-capability-restriction frontmatter field. Under Section 9.2 this is a new opt-in conformance capability scoped to consumers providing the accepted Codex-native agent conversion capability, not behavior-neutral errata: it does not require or imply preservation of any other `config.toml`-native key and does not define behavior for any other conversion target. Implementations not providing Codex-native agent conversion acquire no new required feature. No schema, lockfile version, existing mandatory feature, drift-policy or ownership rule changes. Section 9.3 reviewer approvals and public comment period remain pending; this proposal is not evidence of adoption. Section 8.7, Section 11.3.2, and Appendix C updated. Statement count: 125 -> 126 (121 MUST, 5 SHOULD). | +| 0.1.43 (proposed) | 2026-10-03 | Codex-native agent model preservation and bounded dropped-metadata diagnostic for PR #3150 (closes #3126). Added [req-tg-015] (Section 8.5.1, consumer MUST): a conforming consumer providing the Codex-native agent conversion capability MUST preserve a source-declared `model` or `model_reasoning_effort` string value in its native top-level placement, MUST leave either field absent when absent from the source, MUST diagnose a non-string value by field name without including that value, and MUST emit a diagnostic bounded on both dropped-field count and per-field name length (never the value) for any other dropped, non-capability-restriction frontmatter field. Under Section 9.2 this is a new opt-in conformance capability scoped to consumers providing the Codex-native agent conversion capability, not behavior-neutral errata: it does not require or imply preservation of any other `config.toml`-native key and does not define behavior for any other conversion target. Implementations not providing Codex-native agent conversion acquire no new required feature. No schema, lockfile version, existing mandatory feature, drift-policy or ownership rule changes. Section 9.3 reviewer approvals and public comment period remain pending; this proposal is not evidence of adoption. Section 8.7, Section 11.3.2, and Appendix C updated. Statement count: 125 -> 126 (121 MUST, 5 SHOULD). | Errata (none at publication). diff --git a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md index c68260a80d..9d2bf3e00a 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -493,9 +493,9 @@ Review the requested change. Absent settings stay absent. APM does not translate model names, choose defaults, or validate model availability or supported effort values. -Non-string values are dropped with a diagnostic. Other frontmatter, +Non-string values are dropped with a warning. Other frontmatter, including other native Codex settings and a `codex:` block, is dropped -with a diagnostic; APM provides no arbitrary or namespaced passthrough. +with a warning; APM provides no arbitrary or namespaced passthrough. Remove unnecessary fields or do not rely on their settings in the generated agent. `tools` restrictions still cannot be preserved: the agent may inherit all project/session MCP servers. Remove `tools` only diff --git a/src/apm_cli/integration/agent_integrator.py b/src/apm_cli/integration/agent_integrator.py index 50922eb657..2ca97160fa 100644 --- a/src/apm_cli/integration/agent_integrator.py +++ b/src/apm_cli/integration/agent_integrator.py @@ -516,7 +516,6 @@ def _write_codex_agent( name = name[: -len(".agent")] description = "" body = content - model_fields = AgentIntegrator._CODEX_MODEL_FIELDS model_settings: dict[str, str] = {} fm_match = AgentIntegrator._FRONTMATTER_RE.match(content) @@ -527,7 +526,7 @@ def _write_codex_agent( if isinstance(fm, dict): name = fm.get("name", name) description = fm.get("description", description) - for field in model_fields: + for field in AgentIntegrator._CODEX_MODEL_FIELDS: if field not in fm: continue if isinstance(fm[field], str): @@ -562,16 +561,21 @@ def _write_codex_agent( fields_text = ", ".join(shown_fields) if remaining > 0: fields_text += f" (and {remaining} more)" + singular = len(dropped_field_names) == 1 + field_label = "field" if singular else "fields" + dropped_verb = "was" if singular else "were" + field_reference = "this field" if singular else "these fields" + setting_reference = "its setting" if singular else "their settings" diagnostics.lossy_agent_compilation( message=( f"Codex agent {printable_ascii_text(source.name)}: frontmatter " - f"fields {fields_text} were dropped; " + f"{field_label} {fields_text} {dropped_verb} dropped; " "this metadata is not translated by APM for Codex." ), package=printable_ascii_text(package_name), detail=( - "Fix: remove these fields if unnecessary; otherwise do not rely on " - "their settings in the generated Codex agent." + f"Fix: remove {field_reference} if unnecessary; otherwise do not " + f"rely on {setting_reference} in the generated Codex agent." ), ) else: diff --git a/tests/integration/test_codex_agent_tool_scope_contract.py b/tests/integration/test_codex_agent_tool_scope_contract.py index 36dd293119..dd701c7342 100644 --- a/tests/integration/test_codex_agent_tool_scope_contract.py +++ b/tests/integration/test_codex_agent_tool_scope_contract.py @@ -154,7 +154,7 @@ def test_codex_agent_native_models_and_dropped_metadata_reach_cli_output( assert "[!]" in output assert "1 lossy agent compilation warning" in output assert "model-reviewer.agent.md" in output - assert "fields 'model_verbosity' were dropped" in output + assert "field 'model_verbosity' was dropped" in output assert "not translated by APM for Codex" in output assert "otherwise do not rely on" in output assert "field 'tools' was dropped" not in output diff --git a/tests/spec_conformance/test_manifest_reqs.py b/tests/spec_conformance/test_manifest_reqs.py index d291801692..78a6ae096c 100644 --- a/tests/spec_conformance/test_manifest_reqs.py +++ b/tests/spec_conformance/test_manifest_reqs.py @@ -1261,11 +1261,14 @@ def test_codex_native_model_settings_preserved_and_dropped_metadata_bounded( # inspects frontmatter keys. assert nc_diagnostics.by_category().get(CATEGORY_AGENT_LOSSY_COMPILATION, []) == [] + # The editorial fold removes the undefined "accepted" qualifier and + # explicitly binds the already-tested non-string value exclusion. assert_spec_contains( - "A consumer implementation providing the accepted\nCodex-native agent conversion capability MUST preserve a\nsource-declared", + "A conforming **consumer** implementation providing the\nCodex-native agent conversion capability MUST preserve a\nsource-declared", "MUST leave either field absent from the generated file when it is\nabsent from the source", "This clause does not require or imply\npreservation of any other", "define behavior for any other conversion target. A source-declared\n`model` or `model_reasoning_effort` value that is not a YAML string\nscalar is not subject to preservation under this clause, and the\nconsumer MUST still emit a diagnostic for it.", + "That diagnostic MUST\nidentify the field name but MUST NOT include the rejected value.", "it MUST emit a diagnostic naming the source agent,\nand MUST bound both", "limit (the named-field-name\nlimit MUST be at least one when at least one field was dropped)", "each\nnamed field name MUST be sanitized to printable ASCII (U+0020 through\nU+007E), replacing or escaping any byte outside that range before\ndisplay", diff --git a/tests/unit/integration/test_agent_integrator.py b/tests/unit/integration/test_agent_integrator.py index 28918a0786..dd617b5382 100644 --- a/tests/unit/integration/test_agent_integrator.py +++ b/tests/unit/integration/test_agent_integrator.py @@ -1199,6 +1199,13 @@ def test_codex_agent_target_filename_is_toml(self): {"model_reasoning_effort": "high"}, {"model": "gpt-5.6-sol", "model_reasoning_effort": "high"}, {"model": 'custom"model\\variant', "model_reasoning_effort": "future-effort"}, + pytest.param( + { + "model": 'gpt-4"\nmalicious = "injected', + "model_reasoning_effort": "high\nnew_key = true", + }, + id="embedded-newlines-are-values", + ), ], ) def test_codex_native_settings_reach_generated_agent(self, settings: dict[str, str]) -> None: @@ -1371,6 +1378,10 @@ def test_codex_dropped_field_with_oversized_key_name_is_bounded(self) -> None: assert oversized_key not in message assert "...(truncated)" in message assert len(message) < 300 + assert "frontmatter field " in message + assert "was dropped" in message + assert "remove this field" in warnings[0].detail + assert "its setting" in warnings[0].detail @pytest.mark.parametrize( ("key_count", "expect_elision"),