fix(codex): preserve native agent model settings - #3150
Daniel Meppiel (danielmeppiel) wants to merge 15 commits into
Conversation
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>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes Codex agent conversion so authored native model settings survive conversion to generated .toml, and adds diagnostics/tests/docs to make remaining frontmatter loss explicit.
Changes:
- Preserve
modelandmodel_reasoning_effort(string-only) as top-level TOML keys in Codex agent rendering. - Emit actionable lossy-compilation diagnostics for non-string model settings and for other untranslated frontmatter keys.
- Add unit + CLI integration coverage and update authoring documentation/guide + changelog.
| File | Description |
|---|---|
| src/apm_cli/integration/agent_integrator.py | Preserve native string model settings in Codex TOML output and emit warnings for dropped/untranslated frontmatter. |
| tests/unit/integration/test_agent_integrator.py | Adds focused unit/integration tests for preserved settings, non-string diagnostics, metadata drop diagnostics, and no leakage to other targets. |
| tests/integration/test_codex_agent_tool_scope_contract.py | Extends CLI boundary tests to confirm preserved model settings and surfaced “dropped metadata” warnings. |
| docs/src/content/docs/producer/author-primitives/instructions-and-agents.md | Updates docs/target matrix to reflect preserved Codex model settings and warning behavior. |
| packages/apm-guide/.apm/skills/apm-usage/package-authoring.md | Adds guidance + examples for Codex native model settings and limitations. |
| CHANGELOG.md | Notes behavior change/fix for Codex model settings and dropped-metadata warnings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…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>
…-issue-delivery-3126
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.
PR merge-readiness advisoryBLOCKED at Work retained
Independently checkedThe coordinator verified the live PR head, clean worker checkout, canonical completion JSON Schema, and fresh deterministic owner detection against base Direct execution of the final-head Codex writer with real YAML fixtures confirmed:
These fixture outputs contain only printable ASCII and do not leak field values. Lengths include the fixed fixture's file/package context; they are not universal limits for arbitrary context strings. The worker's persisted log reports 93 passing tests with exact node IDs. Its real source-CLI captures show the long-key truncation and many-key elision. The worker also reports count/length mutation-break checks and a clean full lint mirror. Source-CLI evidence is not frozen-binary or native-runtime evidence. Remaining readiness gaps
Latest observed GitHub stateThe PR is OPEN, non-draft, and MERGEABLE; The correction allowance is exhausted. The worker is stopped and its readiness slot released. Further work requires a new explicit decision; no merge, auto-merge, enqueue, review bypass, or renewed Generated by autopilot-pr-merge-worker. This comment is AI-generated and may contain errors. |
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>
…stic 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 #3126 (comment), coordinator approval request_id 6560378a-9871-43ec-94e1-ccd437a1727a. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…-import coexistence as req-tg-016/017 Adds Section 8.5.9 to the OpenAPM v0.1 spec documenting the already-shipped Cursor-native hook installation behavior from this PR: fail-closed conversion validation (req-tg-016) and Claude-import-coexistence rejection (req-tg-017, both install orders). Narrowly bound to the accepted Cursor-native(+Claude-import) capability only, not a universal target-native obligation. Includes vendor-grounded editorial note (cursor.com/docs/hooks, cursor.com/docs/reference/third-party-hooks) distinguishing APM's own conservative conversion/safety policy from vendor-documented defaults (vendor default is merge-not-reject; vendor top-level shape is not documented as closed). Updates Section 8.7 and 11.3.2 enumerations, Appendix C (2 new rows, total 127 statements / 122 MUST), the requirements manifest, the 0.1.44 (proposed) revision-history row, regenerated CONFORMANCE artifacts, and new drift-sentinel conformance tests (tests/spec_conformance/test_cursor_hook_reqs.py) citing the already-existing, already-passing behavioral integration tests. Folds 4 round-1 findings from the real apm-spec-guardian 4-persona panel (spec-swagger-editor, spec-oci-editor, spec-pkgmgr-editor, spec-tag-architect; synthesizer ship_decision=fold_and_ship, shocked_meter_avg=8.0, 0 blockers across all 4 panels): - req-tg-017: defines the "observable overlap" predicate normatively (non-empty hook-event-identifier intersection after alias normalization), closing a 4/4-panel-convergent second-implementer reproducibility gap. - req-tg-016: adds a SHOULD-level sentence requiring implementations to document/expose their accepted source-format vocabulary, so conformance claims are independently verifiable. - Editorial note: drops the stale "eight" alias count (staleness magnet in informative text). - 0.1.44 revision row: adds a one-line clarification that revision label 0.1.43 and requirement id req-tg-015 are reserved by a concurrent sibling unit (PR #3150) on its own branch, not an unintentional gap. Deferred to v0.1.1 (not folded here, too heavy for a surgical mechanical fold): a machine-readable accepted-vocabulary artifact, and a stale- partial-artifact disposition clause for req-tg-016. Rejected: the stale cursor_preflight_done cache-bypass surface (out of scope, requires a src/apm_cli/** change this unit does not authorize) and the full vocabulary-artifact proposal (superseded by the lighter SHOULD-sentence folded above). No src/apm_cli/** changes. No spec waiver. Statement count unchanged at 127 (122 MUST, 5 SHOULD) -- this fold is prose-only, no new anchors. tests/spec_conformance/ re-verified: 293 passed, 2 skipped (orphan 4-way invariant intact). Closes the Spec conformance gate gap for PR #3149 / issue #3129. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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).
Current bounded result: blocked, not
|
| Panel | Verdict | Shocked | New B | New R | New N |
|---|---|---|---|---|---|
| Swagger / OpenAPI Editor | ship_with_followups | 8/10 | 0 | 2 | 2 |
| OCI Distribution Editor | ship_with_followups | 8/10 | 0 | 2 | 2 |
| Package-Manager Registry-Contract Editor | ship_with_followups | 8/10 | 0 | 1 | 1 |
| W3C TAG Architect | ship_with_followups | 8/10 | 0 | 2 | 2 |
B = new blocking findings, R = new recommended, N = new nits.
Counts are signal strength, not gates. The maintainer ships.
Convergent themes (flagged by 2+ panels)
- T1 -- Non-string source-value fall-through for model/model_reasoning_effort is unspecified (supporting: sw-rec-r1-2, oci-nit-r1-1)
- T2 -- Appendix C section-number ordering disrupted by id-ordered req-tg-015 placement (supporting: sw-nit-r1-2, oci-nit-r1-2, pkg-nit-r1-1, tag-nit-r1-1)
- T3 -- Diagnostic field names are attacker-controlled and need sanitization/redaction guarantees (supporting: oci-rec-r1-1, oci-rec-r1-2)
Fold now (4 item(s))
- [F1 / T1] req-tg-015 -- 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. - [F2 / T3] req-tg-015 -- 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. - [F3 / standalone] Appendix D, row 0.1.43 -- 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. - [F4 / standalone] req-tg-015 -- 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 to v0.1.1 (4 items)
- [F5 / T3] req-tg-015 -- 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.
- [F6 / T2] Appendix C table -- 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.
- [F7 / standalone] req-tg-015 -- 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.
- [F8 / standalone] Appendix D, row 0.1.43 -- 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.
Defer to v0.2
- [F9 / standalone] openapm-v0.1.requirements.yml schema -- 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.
Reserved slot: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.
Rejected findings
- tag-rec-r1-1 -- 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.
Linter handoff: 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.
Orchestrator note on linter handoff: actual post-fold mechanical re-verification (not the prediction above) shows the statement count is unchanged at 126 (121 MUST, 5 SHOULD) across Section 1.3, the Appendix C footer, and the Appendix D 0.1.43 row. F1-F4 landed as additional sentences within the existing req-tg-015 anchor rather than as new anchored MUST statements, so the synthesizer's linter-handoff prediction of a 126 -> 128 increment did not apply to how this fold was structured. All 11 linter checks were re-run against the final head: checks 1, 2, 5, 6, 7, 10 PASS; checks 3, 4, 9 N/A (no schema or fixture touched this fold); check 8 N/A (0 mermaid blocks); check 11 is an advisory note carried over from this PR's separate, already-reviewed diagnostic-fix work unit (.py files were modified there, not in this spec-only fold).
Full per-panel findings
Swagger / OpenAPI Editor -- shocked_meter 8/10, confidence high
Summary: Clean editorial-patch fold. Counts, anchors, conformance-class enumerations, and cross-references are all consistent. Two recommended findings: the bounded-diagnostic clause permits a vacuous zero-limit, and the string-value qualifier leaves the non-string fall-through implicit rather than stated. Two nits on parenthetical clarity and Appendix C section-column ordering. No blocking issues.
New recommended findings (2)
- [sw-rec-r1-1] req-tg-015 -- The bounded-diagnostic clause requires 'MUST bound both (a) the number of dropped field names it names explicitly' to 'a finite, implementation-documented limit'. A degenerate but technically conformant implementation could document a limit of zero, producing a diagnostic that names the source agent but enumerates zero dropped field names. This satisfies the letter of the MUST (the count is bounded and the limit is documented) while defeating the diagnostic's utility. In interface-contract discipline, leaving the floor at zero invites vacuous conformance claims.
Recommended fix: Add a parenthetical after 'a finite, implementation-documented limit' such as '(which MUST be at least one when at least one field was dropped)' to establish a non-vacuous floor. Alternatively, add a SHOULD for naming at least one dropped field when any are dropped, which preserves implementation freedom while signalling design intent. - [sw-rec-r1-2] req-tg-015 -- The preservation clause qualifies the source value as 'string value' ('MUST preserve a source-declared model or model_reasoning_effort string value'), but does not specify behavior when the source declares model or model_reasoning_effort with a non-string YAML type (integer, boolean, sequence, mapping). A non-string value would not match the 'string value' condition, so the preservation MUST would not fire; but those fields also are not capability restrictions under req-tg-006, so they would fall into the residual dropped-field diagnostic bucket. This is arguably correct by exclusion, but a conformance test author reading only req-tg-015 might expect an explicit statement of that fall-through rather than deriving it from the gap between the two clauses.
Recommended fix: Add a single non-normative sentence after 'does not define behavior for any other conversion target': 'A source-declared model or model_reasoning_effort value that is not a YAML string is not subject to preservation under this clause and is handled as any other non-capability-restriction frontmatter field.' This costs zero normative statements and eliminates the interpretive gap.
New nit findings (2)
- [sw-nit-r1-1] The phrase 'in its native top-level placement in the generated Codex agent file' relies on the reader's knowledge of the Codex config.toml schema to know what 'native top-level placement' means. The editorial note's scoping to 'consumers that already provide Codex-native agent conversion' makes this defensible, but a brief parenthetical would remove ambiguity for conformance testers unfamiliar with config.toml layout. -- fix: Change 'in its native top-level placement in the generated Codex agent file' to 'in its native top-level placement (top-level TOML key) in the generated Codex agent file'.
- [sw-nit-r1-2] In the Appendix C table, req-tg-015 (section 8.5.1) is inserted immediately after req-tg-014 (section 8.5.8). The numeric section ordering within the tg block is now non-monotonic: ...8.5.6, 8.5.7, 8.5.8, 8.5.1, 10.4... This is a pre-existing pattern (req-tg-006 is listed at section 8.5 before req-tg-009 at 8.5.1) so it is not a regression, but the new row makes the non-monotonic section column more visible. No action required for this PR; noting for a future Appendix C sort pass.
Preserved strengths confirmed
- Count consistency across Section 1.3, Appendix C footer, and Appendix D revision row is maintained (126 / 121 MUST / 5 SHOULD in all three locations).
- Conformance class enumeration is correct: req-tg-015 appears in Section 8.7 consumer list, Section 11.3.2 consumer enumeration, and Appendix C table with conformance_class consumer.
- Anchor uniqueness and monotonic numbering preserved: req-tg-015 takes the next free tg slot with no renumbering of existing ids.
- Cross-references to req-tg-006 and the Section 3 terminology anchor (Will there be MCP coverage? #3-terminology / capability-restriction) all resolve correctly.
- The three-way partition between req-tg-006 (capability restrictions), req-tg-015 preservation (model/model_reasoning_effort), and req-tg-015 diagnostic (everything else) is cleanly disjoint with no double-coverage.
- All five MUST clauses within req-tg-015 are independently testable by a conformance suite.
OCI Distribution Editor -- shocked_meter 8/10, confidence medium
Summary: The fold is well-scoped and correctly separates model-preservation from capability-restriction semantics. Two recommended findings: diagnostic field names lack a control-character sanitization MUST (terminal injection vector), and the key-name-as-value-channel residual is unacknowledged. Two nits on non-string source handling and Appendix C sort order. No blockers.
New recommended findings (2)
- [oci-rec-r1-1] req-tg-015 -- The diagnostic clause requires naming dropped field names but does not require sanitization of control characters or non-printable bytes in those names before rendering. A malicious package author could craft a YAML field name containing ANSI escape sequences (e.g. ESC[2J to clear screen, or CSI sequences to rewrite terminal lines) that survives the length bound and poisons terminal output or defeats downstream log ingestion sanitizers. This is a realistic supply-chain diagnostic-injection vector.
Recommended fix: Add after 'each to a finite, implementation-documented limit': '; 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'. - [oci-rec-r1-2] req-tg-015 -- The diagnostic MUST NOT include the dropped field's value, but it MUST name the field's key. A malicious package author can encode secret material in the key name itself (e.g. a field named 'password_is_hunter2' or a base64-encoded credential fragment). The length bound caps exposure but does not eliminate it. Combined with the existing count bound this is defense-in-depth, but the spec does not acknowledge this residual channel or recommend that implementations treat diagnostic output as potentially containing sensitive material.
Recommended fix: Add a sentence to the editorial note: 'Implementations SHOULD treat rendered field names as potentially attacker-controlled and apply the same redaction policy they apply to other user-supplied identifiers in diagnostic output.' This keeps it SHOULD-level since the key-name channel is low-bandwidth and bounded.
New nit findings (2)
- [oci-nit-r1-1] The phrase 'string value' in 'MUST preserve a source-declared model or model_reasoning_effort string value' does not specify behavior when the source YAML value is a non-string scalar (e.g. model: 42 or model: true, which YAML 1.2 core schema auto-types). It is unclear whether the consumer must coerce to string, skip preservation, or error. -- fix: Append after 'string value in its native top-level placement': '; if the source value is not a YAML string scalar, the consumer MUST treat it as absent for the purposes of this clause'. Or alternatively define that the value is taken from the post-parse YAML string representation.
- [oci-nit-r1-2] req-tg-015 is appended after req-tg-014 in the Appendix C table, breaking the section-number sort order (8.5.1 appears after 8.5.8). All other req-tg-* entries are in section order. This is cosmetic but diverges from the table's implicit ordering convention. -- fix: Move the req-tg-015 row to sit between req-tg-006 (section 8.5) and req-tg-007 (section 8.5.2) to restore section-order sorting within the tg block.
Preserved strengths confirmed
- Hash envelope anchoring (req-lk-016) and content-addressable lockfile integrity remain intact and unaffected by this fold.
- req-tg-009 fail-closed gate for vocabulary-violating agents is preserved and explicitly scoped separately from req-tg-015.
- The diagnostic bounding pattern (count + per-name length + value exclusion) is a credible defense-in-depth approach consistent with the spec's existing diagnostic discipline.
Package-Manager Registry-Contract Editor -- shocked_meter 8/10, confidence high
Summary: Clean, well-scoped fold that faithfully follows the 0.1.42 opt-in capability pattern. One recommended finding: the Appendix D 0.1.43 revision-history entry should add the explicit negative-scope sentence ('implementations not providing Codex-native agent conversion acquire no new required feature') that the 0.1.42 model carries, to keep the opt-in framing self-documenting in the revision log. No blocking issues; no overlap with existing req-tg-001..014; manifest shape and statement counts are consistent.
New recommended findings (1)
- [pkg-rec-r1-1] Appendix D, row 0.1.43 -- The 0.1.42 Appendix D entry contains the explicit negative-scope sentence 'implementations not claiming it acquire no new required feature,' which is the canonical opt-in capability framing. The 0.1.43 entry omits this sentence. It instead says 'it does not require or imply preservation of any other config.toml-native key and does not define behavior for any other conversion target,' which scopes the positive claim but never explicitly states that consumers NOT providing Codex-native agent conversion acquire no new obligation from this requirement. A future reader scanning revision history could misread the entry as imposing a universal consumer obligation rather than an opt-in capability gated on 'providing the accepted Codex-native agent conversion capability.'
Recommended fix: Append the following sentence to the 0.1.43 Appendix D entry, immediately after the sentence ending '...does not define behavior for any other conversion target': 'Implementations not providing Codex-native agent conversion acquire no new required feature.' This mirrors the 0.1.42 negative-scope framing verbatim and keeps the opt-in pattern self-documenting in the revision log.
New nit findings (1)
- [pkg-nit-r1-1] req-tg-015 is placed after req-tg-014 in the Appendix C table, which is correct by numeric-id convention. However, req-tg-015 maps back to Section 8.5.1 while req-tg-009 (also 8.5.1) sits between req-tg-008 (8.5.3) and req-tg-010 (8.5.4). A reader scanning by section number will find two non-adjacent rows for Section 8.5.1. This is a pre-existing consequence of the id-ordered convention and not introduced by this fold, but a parenthetical note in the table header or a future sort-by-section secondary index would aid auditors.
Preserved strengths confirmed
- req-tg-006 scope boundary is cleanly drawn: the editorial note in req-tg-015 explicitly justifies why model/model_reasoning_effort fall outside capability-restriction scope, preventing any double-coverage ambiguity.
- Manifest entry (openapm-v0.1.requirements.yml) for req-tg-015 matches the shape of sibling entries (id, keyword, section, conformance_class, notes) with no schema drift.
- Statement count arithmetic is correct: 125 -> 126 (121 MUST + 5 SHOULD); all four enumeration surfaces (Section 1.3, Section 8.7, Section 11.3.2, Appendix C table + footer) are updated consistently.
- The opt-in gating phrase 'A consumer implementation providing the accepted Codex-native agent conversion capability' in the req-tg-015 body correctly ensures non-Codex-targeting consumers acquire no new obligation, preserving clean conformance-class separation.
- Diagnostic bounding (dropped-field count and per-field name length each to a finite, implementation-documented limit; value never disclosed) is well-specified and consistent with the bounded-diagnostic pattern established by req-tg-006.
W3C TAG Architect -- shocked_meter 8/10, confidence high
Summary: Clean editorial fold with sound privacy-by-default diagnostic design and effective scope-fencing. Two recommended findings: the layering of a target-specific MUST inside a target-agnostic section creates a precedent that should be addressed structurally in a future revision, and the manifest schema lacks a structured applicability field for conditional requirements. Neither blocks ship.
New recommended findings (2)
- [tag-rec-r1-1] req-tg-015 (Section 8.5.1) -- req-tg-015 is scoped exclusively to the Codex-native conversion target, yet it lives in Section 8.5.1 'Lossy agent conversion' -- a section whose heading and existing requirements (req-tg-006, req-tg-009) are deliberately target-agnostic. This layering mismatch means an implementer reading 8.5.1 as the authoritative surface for all-target lossy-conversion rules will encounter a single-vendor clause mid-stream. A conformance-test generator walking Section 8.5.1 cannot know, from the section heading alone, that one of its requirements applies only to implementations claiming a specific target capability. The editorial note partially mitigates this, but the section-level contract (target-agnostic lossy conversion) is weakened. A future revision adding a second target-specific preservation clause would compound the problem.
Recommended fix: In a future revision (defer-v0.1.x or v0.2), consider factoring target-specific preservation requirements into their own subsection (e.g. 8.5.1.1 'Target-specific field preservation') or into the Target Registry companion, so the generic 8.5.1 remains purely target-agnostic. For this fold, the editorial note's scope disclaimer is sufficient, but the machine-readable manifest entry (see tag-rec-r1-2) should carry an explicit applicability qualifier so a test generator can filter without parsing the prose. - [tag-rec-r1-2] openapm-v0.1.requirements.yml / req-tg-015 -- The requirements manifest entry for req-tg-015 uses the 'notes' field to embed the applicability condition ('a consumer providing the accepted Codex-native agent conversion capability'), but the manifest schema has no structured 'applicability' or 'precondition' field. A conformance-test generator consuming this YAML must parse natural-language notes to determine whether the requirement applies to a given implementation under test. Every other req-tg-* entry in the manifest is unconditionally applicable to the 'consumer' conformance class; req-tg-015 is the first conditionally-applicable entry in that class, and the manifest schema does not distinguish it structurally.
Recommended fix: Add an optional 'applicability' key to the manifest schema (e.g. applicability: 'codex-native-agent-conversion-capability') so that conformance tooling can filter conditional requirements without NLP over the notes field. This is an additive schema change compatible with Section 9.2. Until that key lands, document in the manifest header comment that notes-embedded 'providing the accepted ... capability' phrasing signals conditional applicability.
New nit findings (2)
- [tag-nit-r1-1] In the Appendix C table, req-tg-009 and req-tg-015 both cite section '8.5.1', but every other req-tg-* entry from 008 onward cites a unique subsection number (8.5.3 through 8.5.8). The shared section reference is accurate but makes the table less useful as a locator index -- a reader cannot distinguish which of two 8.5.1-cited requirements they are looking at without following the anchor link.
- [tag-nit-r1-2] The Appendix D revision row says 'opt-in conformance capability' but the normative text uses 'the accepted Codex-native agent conversion capability'. Using 'opt-in' in the changelog versus 'accepted' in the normative text is a minor terminology inconsistency that could confuse a reader cross-referencing the two. -- fix: Align the Appendix D wording to 'consumers providing the accepted Codex-native agent conversion capability' to match the normative clause phrasing.
Preserved strengths confirmed
- Appendix C table and Section 1.3 statement count remain synchronized with the normative body (126 / 121 MUST / 5 SHOULD).
- The machine-readable requirements manifest stays in lockstep with the spec text; the new entry is correctly positioned in the req-tg sequence.
- Section 8.7 and 11.3.2 consumer conformance enumerations both include req-tg-015, maintaining the existing pattern of exhaustive cross-reference.
- The editorial note's explicit scope disclaimer ('does not introduce a new vendor-namespaced metadata system... does not standardize dropped-metadata handling for any other target') is well-crafted defensive fencing against scope creep.
- The diagnostic's MUST-NOT-include-value clause is a sound privacy-by-default design -- it prevents accidental leakage of model configuration secrets through diagnostic output.
This panel is advisory. It does not block merge. Re-apply the spec-review label after addressing feedback to re-run.
Orchestrator correction (post-advisory, same work unit):
Two evidence gaps were raised against this advisory after it was posted and have now been addressed honestly, not glossed over:
-
Synthesizer schema validation: FAILS, not all-5-pass. The synthesizer's raw JSON return does NOT validate against
synthesizer-return-schema.json. Itsdefer_v0_2[0]item (the F9 entry above) is rejected withAdditional properties are not allowed ('reserved_slot_anchor' was unexpected). Root cause: the schema'sfold_itemdefinition setsadditionalProperties: falsescoped to only its own 5 properties, thendefer_v0_2composes it viaallOfwith a second subschema that requiresreserved_slot_anchor-- a standard JSON-SchemaallOf+additionalProperties:falseanti-pattern, since eachallOfbranch is validated independently against the full instance. This was manually confirmed to be a schema-authoring defect, not a content defect: the synthesizer's actual F9 item has all requiredfold_itemfields correctly typed, anidmatching^F[1-9][0-9]*$, and a valid stringreserved_slot_anchor; validating it against barefold_itemalone (without the conflicting second branch) passes. The schema file is a skill asset outside this work unit's authorized scope and was NOT edited. This gap is reported, not resolved, pending a maintainer decision on the schema itself. -
Non-Codex deployment test coverage: now genuinely covered (was vacuous). The conformance test's prior "different (non-Codex) target is unaffected" assertion only wrote a fixture file and read it back against itself, never invoking any deployment entrypoint (0 calls to
integrate_agents_for_target). It has been replaced with a real call toAgentIntegrator().integrate_agents_for_targetagainst theclaudetarget (which deploys viacopy_agent, a verbatim copy with no frontmatter filtering), asserting the deployed file is byte-identical to the source -- including themodel,model_reasoning_effort, and unsupportedmodel_verbositykeys -- and that no req-tg-015 lossy-compilation diagnostic fires for this target. Mutation-tested: disabling the deployment dispatch made the new assertion fail (0 != 1); restored and reverified passing. Test-only change, nosrc/**modified. Pushed atfc652ea9f4b709a120cc635b1844ce017f0e1f7a. Fulltests/spec_conformancesuite: 292 passed, 2 skipped (pre-existing). Full current-main lint-mirror chain (ruff check, ruff format --check, pylint R0801, auth-signal lint) re-run clean at this head.
Status unchanged by this correction: the repository's Mode B spec-conformance gate failure for src/apm_cli/integration/agent_integrator.py (substantive critical-path lines without a spec citation) is a separate, already-reported classification question for a maintainer to decide -- it is not resolved, waived, or newly justified by this correction. mergeStateStatus remains BLOCKED; the sergio-sisternes-epam review request remains outstanding and untouched. No ship_now, merge, or auto-merge action is implied by this note.
Schema-and-report correction (schema-report-20261005T091021Z-pr3150, same work unit, separate authorization):
Two more corrections to this advisory, verified independently against live state before posting:
-
Synthesizer schema: now FIXED, not FAILS. The previous correction above accurately reported the schema rejected a valid
defer_v0_2entry via a Draft 7allOf+additionalProperties:falseanti-pattern. That schema defect has now been repaired under a separately authorized, narrowly scoped unit:defer_v0_2.itemsno longer composesfold_itemviaallOf; it now$refs a new definition,fold_item_with_reserved_slot, which inlines all offold_item's properties plusreserved_slot_anchorunder oneadditionalProperties:false/required.fold_itemitself (used byfold_now/defer_v0_1_1) is unchanged. Both the canonical (.apm/) and deployed (.agents/) copies of the schema now validate and are byte-identical (sha256:2115c15b...);apm audit --cireports no drift and all 10 policy checks pass. 12 new regression tests were added (tests/unit/test_apm_spec_guardian_synthesizer_schema.py), including load-bearing mutation tests proving each new guard (thereserved_slot_anchorrequirement, andadditionalProperties:falseover the full 6-key shape) is actually necessary, a before/after proof that the original synthesizer return for this PR now passes where it previously failed, and a byte-identity check between the canonical schema and its deployed mirror. The onlyapm.lock.yamlchanges are the two permitted hash fields for this one file (deployments[].content_hashand the matchinglocal_deployed_file_hashesentry);uv.lockis untouched. -
F9 disposition corrected. F9 ("add an optional
applicabilitykey to the requirements manifest schema") hadreserved_slot_anchor: "Section 9.2 ...". Section 9.2 is change-classification policy prose (how to decide if a spec change is breaking), not a genuine reserved-for-v0.2 slot -- the spec's actual reserved v0.2 anchors are Section 4.8 (workspaces), Section 7.9 (version withdrawal), Section 10.12 (publisher attestations), and Appendix B (registry HTTP API). F9 does not belong under any of those. Running the one authorized targetedspec-editor-synthesizercorrection call against the four retained panelist returns and the original synthesis, F9 has been reclassified fromdefer_v0_2intodefer_v0_1_1: it is a small, additive, non-breaking manifest-schema extension suited to a patch-level spec revision, not an architectural surface requiring coordinated v0.2 work. The corrected full report (with F9 moved and all other findings and the overallship_decisioncarried through unchanged) validates against the fixed schema. This reclassification affects only how F9 is tracked for a future revision; it does not itself change any normative spec text, introduce a waiver, or decide the broader applicability-field question -- that decision remains a maintainer call.
Independently re-verified live state at this write (not restating a prior claim): PR head fc652ea9f4b709a120cc635b1844ce017f0e1f7a plus two new commits (a875a77122, 4b7e4118bb) containing only the schema fix, its tests/fixture, a documentation clarification, and the regenerated mirror/lock-hash pair -- src/apm_cli/** is untouched by this correction. gh pr checks right now shows 22 total rollups: 19 SUCCESS, 2 NEUTRAL, 1 SKIPPED, 0 FAILURE, 0 QUEUED -- including Spec conformance gate: SUCCESS. The combined focused test run (tests/unit/test_apm_spec_guardian_synthesizer_schema.py + tests/spec_conformance/) is 304 passed, 2 skipped (pre-existing, unrelated). The full lint-mirror chain (ruff check, ruff format --check, pylint R0801, lint-auth-signals.sh, lint-architecture-boundaries.sh, plus the three pure-grep CI guards) was re-run clean at this head. mergeStateStatus remains BLOCKED and reviewDecision is empty: the outstanding review request for sergio-sisternes-epam is untouched and this correction does not request, dismiss, or substitute for it. No ship_now, merge, auto-merge, waiver, or new normative spec change is implied by this note; the broader applicability-field question and any remaining non-CI readiness items stay open for a maintainer to decide.
Generated by apm-spec-guardian. This comment is AI-generated and may contain errors.
…-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>
…radiction 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>
…ile 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>
…ndary 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>
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>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| CLI Logging Expert | 0 | 0 | 1 | Diagnostic messages are well-structured, bounded, sanitized, and actionable; one minor grammar nit on singular/plural agreement. |
| DevX UX Expert | 0 | 0 | 1 | Diagnostic wording follows the APM pattern (what/why/fix); happy path stays quiet; docs and skill resource updated. Ship. |
| Doc Writer | 0 | 1 | 0 | Authoring and contributor guidance match the source; correct the changelog's already-shipped claim for this unreleased fix. |
| Python Architect | 0 | 0 | 1 | Clean single-owner model-field extraction with bounded diagnostics; Draft 7 schema fix structurally guarded. Ship. |
| Supply Chain Security | 0 | 0 | 1 | Lockfile hashes consistent; untrusted YAML metadata reaches TOML via controlled keys and library-safe serialization; diagnostics bounded, sanitized, value-free. No supply-chain concerns. |
| Test Coverage | 0 | 1 | 1 | All four critical Codex-model-preservation surfaces have real-binary e2e regression traps plus thorough unit boundary coverage; schema-fix tests are solid. No blocking gaps. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Doc Writer] CHANGELOG Changed entry says 'documenting the already-shipped' for an unreleased fix. -- Factual error in published text: the preservation behavior ships in this PR, not a prior release. Replace 'already-shipped' with wording that matches the proposed 0.1.43 amendment status. One-line fix.
- [spec-editor-synthesizer] Spec fold F4 references a Target Registry companion that has no published artifact in the repository. -- The synthesis proposes editorial text claiming model/model_reasoning_effort are 'documented in the OpenAPM Target Registry companion for the Codex target entry,' but no companion document exists as a file. Either publish the companion first, or soften to 'documented in Codex host documentation' to avoid an unverifiable normative cross-reference. F1-F3 remain sound and should be applied.
- [spec-editor-synthesizer] Spec fold F5 maps req-tg-015 to Section 10.11 row 16 (capability-scope widening), but the requirement covers metadata preservation and diagnostic safety. -- req-tg-015 is about model-setting preservation and bounded diagnostic output, not capability-scope widening. Adding it to row 16 conflates metadata loss with capability broadening. If the diagnostic-safety surface warrants a threat-table entry, it should be a distinct row or deferred, not grafted onto row 16.
- [Test Coverage] No scenario-evidence row for the synthesizer-return-schema Draft 7 fix and its 10 regression tests. -- The schema fix changes observable behavior (previously rejected valid defer_v0_2 entries now validate) and has 10 dedicated tests including before/after proof and mutation guards. The PR body's scenario table omits them. Lower priority since the tests are thorough and the surface is internal tooling.
- [Supply Chain Security] Add an embedded-newline parametrize case to the model-settings round-trip test for self-documenting TOML injection resistance. -- The toml library handles newlines correctly and the existing tomllib round-trip catches malformed output, but a newline-injection case makes the threat vector visible without relying on implicit library trust. Evidence: existing test passes on secure-by-default surface.
Architecture
classDiagram
direction LR
class BaseIntegrator {
<<Base>>
+init_link_resolver()
+find_agent_files()
+_check_adopt_or_skip()
#_LF_NORMALIZED_DEPLOY bool
}
class AgentIntegrator {
<<Subclass>>
+integrate_agents_for_target(target, package_info, project_root) IntegrationResult
-_write_codex_agent(source, target, diagnostics, package_name)
-_display_dropped_field_key(field) str
-_warn_codex_tools_dropped(diagnostics, source, package_name)
-_CODEX_MODEL_FIELDS tuple
-_CODEX_KNOWN_FIELDS frozenset
-_MAX_DROPPED_FIELDS_SHOWN int
-_MAX_DROPPED_FIELD_KEY_LEN int
}
class DiagnosticCollector {
<<Collect-then-Render>>
+lossy_agent_compilation(message, package, detail)
+warn(message, package)
+by_category() dict
+render_summary()
}
class TargetProfile {
<<ValueObject>>
+root_dir str
+primitives dict
+auto_create bool
}
class IntegrationResult {
<<ValueObject>>
+files_integrated int
+target_paths list
}
class printable_ascii_text {
<<Pure>>
+__call__(text) str
}
BaseIntegrator <|-- AgentIntegrator : extends
AgentIntegrator ..> DiagnosticCollector : pushes diagnostics
AgentIntegrator ..> TargetProfile : reads target config
AgentIntegrator ..> IntegrationResult : returns
AgentIntegrator ..> printable_ascii_text : sanitizes field names
note for AgentIntegrator "Single-owner: _CODEX_MODEL_FIELDS is\ncanonical for model-field extraction\n+ dropped-field exclusion set"
class AgentIntegrator:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm install --> integrate_agents_for_target()
src/apm_cli/integration/agent_integrator.py:139"] --> B{mapping.format_id\nagent_integrator.py:274}
B -- codex_agent --> D["_write_codex_agent()\nagent_integrator.py:496"]
B -- kiro_agent --> K["[FS] _write_kiro_agent()\nagent_integrator.py:756"]
B -- other --> V["[FS] copy_agent() verbatim\nagent_integrator.py:287"]
D --> E["[I/O] source.read_text(encoding=utf-8)\nagent_integrator.py:512"]
E --> F{_FRONTMATTER_RE.match\nagent_integrator.py:522}
F -- No match --> AA
F -- Match --> H["load_yaml_str(frontmatter)\nagent_integrator.py:525"]
H -- YAMLError --> EX["_warn_codex_unverified_scope()\nagent_integrator.py:594"]
EX --> AA
H -- OK --> I{isinstance fm dict?\nagent_integrator.py:526}
I -- No --> J["_warn_codex_unverified_scope()\nagent_integrator.py:582"]
J --> AA
I -- Yes --> L["fm.get name + description\nagent_integrator.py:527-528"]
L --> M["for field in _CODEX_MODEL_FIELDS\nagent_integrator.py:529"]
M --> N{fm.field exists\nand isinstance str?}
N -- "str" --> O["model_settings.field = fm.field"]
N -- "non-str" --> P["diagnostics.lossy_agent_compilation\n'must be a string and was dropped'\nagent_integrator.py:536"]
N -- "absent" --> Q["skip -- omission = absent in TOML"]
O --> R
P --> R
Q --> R
R["dropped = fm.keys minus _CODEX_KNOWN_FIELDS\nagent_integrator.py:551"] --> S{dropped non-empty?}
S -- Yes --> T["bound shown to _MAX_DROPPED_FIELDS_SHOWN\n_display_dropped_field_key: printable_ascii_text\n+ truncate to _MAX_DROPPED_FIELD_KEY_LEN\nagent_integrator.py:553-575"]
T --> U["diagnostics.lossy_agent_compilation\n'fields X were dropped; not translated'"]
U --> W
S -- No --> W{tools in fm?\nagent_integrator.py:586}
W -- Yes --> Y["_warn_codex_tools_dropped()\nagent_integrator.py:587"]
W -- No --> AA
Y --> AA
AA["build doc = name + description\n+ **model_settings + developer_instructions\nagent_integrator.py:601-605"] --> AB["[FS] write_text_lf target toml.dumps doc\nagent_integrator.py:606"]
Recommendation
Ten panelists and a four-panel spec review converge at zero blocking findings. The Codex model-preservation fix is well-bounded, thoroughly tested (97 current-head cases, 304 spec-conformance cases), and CI-green on all required checks. Correct the CHANGELOG already-shipped wording before merge; the remaining nits (plural grammar, terminology sync, alias style, scenario labels, injection test case) are post-merge cleanup. For the spec fold: apply F1-F3 which are mechanically sound; hold F4 pending companion-document publication or wording adjustment, and F5 pending a precise threat-row mapping. The highest-signal followup to track is the CHANGELOG factual correction.
Full per-persona findings
Python Architect
- [nit] Local alias style inconsistency: model_fields at L519 vs full AgentIntegrator._CODEX_KNOWN_FIELDS at L551 in the same method at
src/apm_cli/integration/agent_integrator.py:519
Inside _write_codex_agent, _CODEX_MODEL_FIELDS is aliased to a local (model_fields = AgentIntegrator._CODEX_MODEL_FIELDS, L519) for the extraction loop, but _CODEX_KNOWN_FIELDS (L551) and the MAX_DROPPED* constants (L555, L557) are referenced via the full class path. Either alias all class constants or alias none for uniform readability within the method. Trivially skippable.
Design patterns
- Used in this PR: Base class + subclass -- AgentIntegrator extends BaseIntegrator; new model-field extraction and bounded-diagnostic logic correctly lives in the Codex-specific subclass, not the shared base. Annotated as <> / <> in the class diagram.
- Used in this PR: Collect-then-render -- DiagnosticCollector.lossy_agent_compilation() accumulates diagnostics during the transform; render_summary() displays them after all agents are processed. Annotated as <> in the class diagram.
- Used in this PR: Dataclass-as-value-object -- _CODEX_MODEL_FIELDS (tuple) and _CODEX_KNOWN_FIELDS (frozenset) are immutable class-level vocabularies serving as the single canonical authority for field classification. Annotated via note-for-AgentIntegrator in the class diagram.
- Pragmatic suggestion: none -- the current shape is the simplest correct design at this scope. The inline extraction + diagnostic sequence inside _write_codex_agent is readable and well-commented; extracting a helper would be warranted only when a third field-category (beyond model fields and tools) arrives.
CLI Logging Expert
- [nit] Dropped-fields diagnostic uses plural grammar even when exactly one field is dropped. at
src/apm_cli/integration/agent_integrator.py:568
Line 568 always emits 'fields {X} were dropped' and the detail always says 'remove these fields ... their settings'. When only one unsupported field is present (e.g. 'model_verbosity' alone, as exercised by the integration test at test_codex_agent_tool_scope_contract.py:157), the rendered message reads 'fields 'model_verbosity' were dropped' -- grammatically incorrect. Good CLI output matches count to noun/verb ('field ... was dropped' vs 'fields ... were dropped'); npm, pip, and cargo all handle this. The non-string diagnostic at line 539 already does this correctly ('field ... was dropped'). A one-liner ternary on len(dropped_field_names) would fix both the message and detail.
Suggested: Branch on len(dropped_field_names): singular 'field {X} was dropped' + 'this field' / 'its setting' when 1, plural 'fields {X} were dropped' + 'these fields' / 'their settings' when >1. Update the integration test assertion at line 157 to match.
DevX UX Expert
- [nit] Skill resource says 'diagnostic' where the CLI renders 'warning' at
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md:496
package-authoring.md uses 'dropped with a diagnostic' while instructions-and-agents.md says 'dropped with a warning' and the CLI renders '[!] N lossy agent compilation warning(s)'. The user-facing term is 'warning'; the skill resource should match so an agent consuming the resource echoes the same vocabulary the user sees in their terminal. Rule 4 (skill resources stay in sync with docs) flags this.
Suggested: Change 'Non-string values are dropped with a diagnostic. Other frontmatter, including other native Codex settings and acodex:block, is dropped with a diagnostic' to use 'warning' in both places, matching the docs page and the CLI output.
Supply Chain Security
- [nit] No explicit test for TOML structural-injection via newline/quote in model setting values. at
tests/unit/integration/test_agent_integrator.py:1197
The test_codex_native_settings_reach_generated_agent parametrization covers quotes and backslashes ('custom"model\variant') but not embedded newlines, which are the canonical TOML structural-injection vector. The toml library handles this correctly (verified manually: round-trip with 'gpt-4"\nmalicious = "injected' passes), and the tomllib.loads() round-trip in the existing test would catch malformed output for any value tested. Adding a parametrize case with an embedded newline (e.g. 'model\ninjected_key = true') would make the injection-resistance property self-documenting in the test suite without relying on implicit library trust.
Suggested: Add a parametrize case like {"model": "gpt-4"\nmalicious = "injected", "model_reasoning_effort": "high\nnew_key = true"} to test_codex_native_settings_reach_generated_agent. The existing tomllib.loads() round-trip assertion already proves correctness; this just makes the threat vector visible in the test.
Proof (test passed):tests/unit/integration/test_agent_integrator.py::tests/unit/integration/test_agent_integrator.py::TestCodexAgentIntegration::test_codex_native_settings_reach_generated_agent-- proves: Codex agent model settings survive YAML-to-TOML round-trip without corruption or injection for tested value shapes. [secure-by-default]
assert tomllib.loads(target.read_text(encoding='utf-8')) == {'name': 'reviewer', 'description': 'Review code', 'developer_instructions': 'Review changes.', **settings}
Doc Writer
- [recommended] Describe the Codex preservation fix as unreleased, not already shipped. at
CHANGELOG.md:12
The new Changed entry calls model/model_reasoning_effort preservation and bounded dropped-metadata diagnostics 'already-shipped', but the supplied base-to-head diff introduces those paths in AgentIntegrator._write_codex_agent, and the adjacent Fixed entry records them under Unreleased. Implementation on this PR branch is not evidence of a published release. The specification also labels amendment 0.1.43 proposed and explicitly says its review process remains pending. The release note should distinguish the new implementation from the proposed normative contract so readers of the released CLI do not assume this fix is already available.
Suggested: Replace 'documenting the already-shipped' with 'proposing a conformance requirement for the' and remove the redundant trailing 'as a normative conformance requirement'. Keep the behavior fix under Unreleased; do not assert a released version without release evidence.
Test Coverage
- [nit] Scenario Evidence row 2 labels test_codex_native_settings_reach_generated_agent as 'integration' but it is unit-tier (Python API call, not CLI invocation). at
tests/unit/integration/test_agent_integrator.py:1204
The test at tests/unit/integration/test_agent_integrator.py::TestCodexAgentIntegration::test_codex_native_settings_reach_generated_agent calls AgentIntegrator().integrate_agents_for_target() directly in Python, not through the real apm binary. This is unit-tier evidence (mocked CLI boundary). The Scenario Evidence table row 2 labels it 'integration'. Per the tier-floor matrix, this surface (install pipeline behavior: omitted/individual model settings) has a floor of integration-with-fixtures. The actual e2e coverage for this surface exists in test_codex_agent_tool_scope_is_never_silently_lost (negative assertions: 'model' not in unscoped_agent) and the new test_codex_agent_native_models_and_dropped_metadata_reach_cli_output (positive assertions), both of which drive the real binary. So the floor IS met elsewhere -- the mislabel is cosmetic, not a coverage gap.
Suggested: Correct the Scenario Evidence table row 2 Type column from 'integration' to 'unit', or add a parenthetical noting the e2e coverage is in test_codex_agent_tool_scope_contract.py.
Proof (test passed):tests/unit/integration/test_agent_integrator.py::TestCodexAgentIntegration::test_codex_native_settings_reach_generated_agent-- proves: Omitted or individually supplied model/effort settings stay exactly as authored in the generated Codex TOML [multi-harness-support,portability-by-manifest]
assert tomllib.loads(target.read_text(encoding='utf-8')) == {'name': 'reviewer', 'description': 'Review code', 'developer_instructions': 'Review changes.', **settings} - [recommended] Scenario Evidence table has no row for the synthesizer-return-schema Draft 7 fix and its 10 regression tests. at
tests/unit/test_apm_spec_guardian_synthesizer_schema.py
The PR includes a non-trivial behavioral change: repairing the apm-spec-guardian synthesizer-return-schema so that valid defer_v0_2 entries (with reserved_slot_anchor) no longer fail validation under Draft 7's additionalProperties:false semantics. 10 focused regression tests are added in tests/unit/test_apm_spec_guardian_synthesizer_schema.py, including before/after proof, negative guards, load-bearing proof pattern, and a structural-drift parity guard. The PR body mentions these in prose (the 'closed schema/report unit') and in the historical evidence row, but does not map them through the scenario-evidence rubric (no scenario row with principle mapping). Per the rubric, every behavioral change should appear in at least one scenario row; the schema fix changes observable behavior (a valid synthesizer return that was previously rejected now validates) and has dedicated tests. The surface is internal (not user-CLI-facing) so this is recommended, not blocking -- the tests themselves are well-targeted and sufficient.
Suggested: Add a scenario evidence row, e.g.: '| 5 | Schema accepts valid defer_v0_2 entries that Draft 7 allOf previously rejected | OSS | tests/unit/test_apm_spec_guardian_synthesizer_schema.py::test_valid_full_report_and_original_return_fail_broken_schema_pass_fixed | unit |'
Proof (test passed):tests/unit/test_apm_spec_guardian_synthesizer_schema.py::test_valid_full_report_and_original_return_fail_broken_schema_pass_fixed-- proves: The repaired synthesizer-return schema accepts a valid defer_v0_2 entry with reserved_slot_anchor that the original allOf composition wrongly rejected under Draft 7 [oss]
jsonschema.validate(instance, fixed_schema) # passes; with pytest.raises(jsonschema.ValidationError): jsonschema.validate(instance, broken_schema) # fails
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
This is the fresh 2026-10-06 full engineering panel, not terminal acceptance. All genuine in-scope findings, including nits, will be folded before delta review. Local complete lint mirror passed at this head. Earlier 97-case tests are historical; exact-head tests are being rerun. Provider scanning/approval/queue requirements are not waived.
Historical corrected advisory, preserved unchanged
Readiness blocked: required CodeQL analysis missing
Head: 24f18c7d3f7be1269381fe15a936521fb1866c65; main: 18c4c43c924ceae890fe0f2038806690e5b2d6c8. There is no accepted terminal ship_now or ready-to-merge receipt.
The renewed run has now closed several evidence gaps:
- A genuine full-schema spec synthesis received the complete repaired schema, four genuine retained panelist returns and original synthesis. The actual inputs and full output were independently matched.
- Genuine architect, coverage and documentation reviews covered the previously unreviewed
4b7e4118..24f18c7dchanges. The corrected general CEO returnedship_with_followups; its abbreviated reviewer inputs are not verbatim full originals, and its administrative-bypass suggestion is not adopted. - The unmasked current-head focused command passed 97 cases: 95 agent-unit cases plus two real CLI integration cases, including settings placement, absent fields, other-target preservation and diagnostics. The blocked completion now schema-validates and its owner report independently matches; the blocked-status verifier skips terminal requirements, so its exit 0 is not a readiness pass.
The fresh no-bypass provider read is conclusive: MERGEABLE, protected BLOCKED, merge requirements UNMERGEABLE. Its failed Repo-rules condition says "Code scanning is still expecting 1 result from CodeQL for 83e9d6b or 24f18c7." This is a separate code_scanning rule, not a required status-check context named CodeQL. The ordinary required gate check passes. A neutral summary and successful standard analysis jobs do not supply the missing historical analysis, nor does the missing analysis demonstrate a new vulnerability. Restoration is separately blocked on package-read authorization; offline database preparation is not a scan or upload.
CODEOWNER review, last-push approval and the protected SQUASH merge queue remain separate gates; sergio-sisternes-epam is untouched. Earlier premature recommendations, the literal-filename PATCH failure, invalid receipts and execution/provenance qualifications remain preserved as history. The closed 4b7e4118 attempt is not retroactively successful. No merge, auto-merge, reviewer bypass, scanning-rule waiver or broader applicability-field decision is authorized.
Generated by autopilot-pr-merge-worker. This comment is AI-generated and may contain errors.
Generated by autopilot-pr-merge-worker. This comment is AI-generated and may contain errors.
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>
APM Spec Guardian:
|
| Panel | Stance | Shocked | New B | New R | New N |
|---|---|---|---|---|---|
| spec-swagger-editor | ship | 8 | 0 | 0 | 0 |
| spec-oci-editor | ship | 8 | 0 | 0 | 0 |
| spec-pkgmgr-editor | ship_with_followups | 8 | 0 | 0 | 0 |
| spec-tag-architect | ship | 8 | 0 | 0 | 0 |
B = new blocking findings, R = recommended, N = nits. Counts are signal strength, not gates.
Convergent themes (flagged by 2+ panels)
-
T1 -- Drafter F4/F5 alternatives independently confirmed architecturally superior to original synthesis proposals by all four panels: opaque-value wording avoids unverifiable companion cross-reference (F4), standalone Section 10.11 paragraph avoids threat-surface conflation with row 16 (F5) (tag-rec-r1-1, tag-rec-r1-3, oci-nit-r1-1)
-
T2 -- Statement count (126/121/5), anchor uniqueness (127 total, 126 concrete), and cross-reference accuracy preserved through all round 2 folds without drift across Section 1.3, Appendix C, Appendix D, and requirements manifest (sw-rec-r1-1, oci-rec-r1-1)
Prior findings not adopted
-
pkg-rec-r1-2 -- Round 1 reject preserved. Blanket MUST-NOT prohibition against consumers preserving any other Codex-native key exceeds bounded req-tg-015 scope. The declarative non-extension sentence is sufficient without a new normative constraint. Round 2 pkgmgr panel confirms: 'rejected with acceptable rationale -- the declarative non-extension sentence is sufficient without converting to a MUST-NOT prohibition that would change character.'
-
tag-rec-r1-4 -- Round 1 reject preserved. Finding stated 'No text change needed in this PR' -- a v0.2 structural observation about per-target normative prose accretion with no foldable patch. Round 2 tag panel confirms: 'deferred_clean -- finding explicitly stated no text change needed; synthesis correctly rejected as having no actionable fold.'
-
sw-nit-r1-1 -- Round 1 reject preserved. Observational dual-anchor nit with no proposed fix; both panels (swagger and pkgmgr) acknowledge the pattern matches established req-tg-006 precedent. Splitting the anchor would require renumbering, classified as breaking under Section 9.2.
-
pkg-nit-r1-2 -- Round 1 reject preserved. Same dual-anchor observation as sw-nit-r1-1; no fix proposed. Pattern established by req-tg-006 and not a regression introduced by this PR.
-
sw-nit-r1-2 -- Round 1 reject preserved. Observational navigability concern about capability-restriction cross-reference link target; no fix proposed. Slug-based heading cross-reference convention is spec-wide and consistent with all other internal links.
-
pkg-nit-r1-1 -- Round 1 reject preserved. Parenthetical '(top-level TOML key)' deliberately retained to aid implementers unfamiliar with Codex config.toml layout; tag panel independently confirmed it aids readers.
-
tag-nit-r1-2 -- Round 1 reject preserved. Same parenthetical observation as pkg-nit-r1-1; tag panel itself noted the parenthetical 'aids implementers' while calling it mildly redundant.
-
oci-nit-r1-2 -- Round 1 reject preserved. No fix proposed. Spec deliberately leaves replacement/escaping mechanism for non-printable-ASCII bytes implementation-defined. Round 2 OCI panel confirms: 'oci-nit-r1-2 rejected with an acceptable rationale that implementation-defined escaping is intentional.'
-
tag-nit-r1-1 -- Round 1 reject preserved. Finding stated 'No action needed; noted for completeness.' Appendix C section reference (8.5.1) is correct and consistent with req-tg-009.
Linter notes
- Checks 1-10 pass. Check 11 notes Python changes outside the spec surface; the actual general panel and complete lint mirror cover them.
Linter handoff: No new folds to verify. The Wave 5 linter already passed all 10 normative checks on head d298a41 (spec-linter.json). Statement count 126 (121 MUST, 5 SHOULD) verified at Section 1.3 line 139, Appendix C footer, and Appendix D row 0.1.43. Anchor count 127 (126 concrete + 1 template req-XXX). req-tg-015 appears at 7 locations (anchor, body, Section 8.7, Section 10.11 note, Section 11.3.2, Appendix C, Appendix D). Zero matches for 'accepted.*Codex' across normative body, Appendix D, and requirements manifest (F2 propagation complete). 88 instances of 'A conforming consumer' including exactly 1 for req-tg-015 (F1 applied). All five mutation guards confirmed effective at exit 1. Check 11 (advisory) confirmed non-spec Python changes were reviewed by the genuine general panel and full lint mirror. No special handoff needed.
Full per-panel findings
spec-swagger-editor -- shocked_meter 8/10, confidence high
Both round-1 recommended findings cleanly closed: 'A conforming consumer implementation' convention restored (F1), undefined 'accepted' qualifier removed from all three propagation sites (F2). F3 closes the non-string-diagnostic value-exclusion gap with two additional MUST clauses. The drafter chose superior alternatives to the flagged F4 and F5 proposals: opaque-value wording avoids an unverifiable companion provenance claim, and a separate Section 10.11 paragraph correctly distinguishes the diagnostic-safety surface from row 16's capability-scope widening. Count consistency (126/121/5), anchor uniqueness, cross-references, and class enumeration all verified on d298a41 (4216 lines). No new findings.
Both round-1 recommended findings are cleanly closed. sw-rec-r1-1 (missing 'conforming' qualifier and bold class-name markup) is closed by F1; sw-rec-r1-2 (undefined 'accepted' qualifier) is closed by F2 plus F2 propagation to Appendix D 0.1.43 and the requirements manifest.
No new findings.
Preserved strengths confirmed
-
Count consistency across Section 1.3, Appendix C trailer, and Appendix D revision-history row 0.1.43 is perfect (126 statements, 121 MUST, 5 SHOULD) -- verified by comparing 127 inline anchors minus 1 template req-XXX against the three count sites. No drift introduced by the round 2 folds.
-
Anchor uniqueness and monotonic numbering remain intact: exactly one anchor exists; req-tg-015 takes the next free slot after req-tg-014 with no renumbering.
-
Cross-reference accuracy: all six req-tg-015 references (anchor at line 2688, Section 8.7 at line 2997, Section 10.11 paragraph at line 3383, Section 11.3.2 at line 3644, Appendix C at line 4107, Appendix D at line 4179) resolve correctly and cite consistent section/class/keyword metadata.
-
RFC 2119 keyword discipline within req-tg-015 remains clean after F3 fold: all normative claims (MUST preserve, MUST leave, MUST emit, MUST identify, MUST NOT include, MUST bound, MUST be at least one, MUST be sanitized) use uppercase; non-normative prose uses lowercase phrasing ('does not require', 'does not define', 'does not prescribe').
-
The editorial note correctly distinguishes req-tg-006 (capability-restriction scope) from req-tg-015 (model metadata scope) without introducing normative overlap.
-
Conformance class assignment (consumer) is correct in all enumeration sites: Appendix C, Section 8.7, Section 11.3.2, Appendix D, and the requirements manifest.
spec-oci-editor -- shocked_meter 8/10, confidence high
All three round-1 findings closed. The value-exclusion clause (F3) is correctly placed and normatively binding. The Section 10.11 separate note is technically sounder than the rejected F5 row-16 graft. The editorial note correctly declares values opaque and ordering unspecified without inventing a companion reference (F4 wisely withheld). No hash, content-addressing, mirror-tolerance, extraction, or provenance regressions. Ship-clean from the distribution and supply-chain lens.
The single round-1 recommended finding (oci-rec-r1-1, value-exclusion gap for non-string diagnostics) is fully closed. Both nits are resolved: oci-nit-r1-1 addressed by a separate Section 10.11 note rather than a row-16 graft (technically sounder), and oci-nit-r1-2 rejected with an acceptable rationale that implementation-defined escaping is intentional. Closure rate: 3/3 resolved.
No new findings.
Preserved strengths confirmed
-
Hash envelope anchoring (req-lk-016) remains well-specified with algo:hex form, bare-hex deprecation horizon, and enumerated allowed algorithms (sha256/sha384/sha512). No changes in this revision.
-
Canonical git tree hash construction (Section 5.6.4) retains its mode/name/blob-sha256 line format, lexicographic sort, and recursive subdirectory definition. Untouched by the Codex-scoped req-tg-015 addition.
-
Mirror tolerance (req-rs-009) correctly anchors trust on resolved_hash, not resolved_url, and the editorial note requiring verbatim byte replication remains. No changes.
-
Fail-closed archive extraction (req-sc-004) retains media-type pinning (application/gzip over tar), decompression cap (100 MB default), entry-count cap (10000 default), and zip-slip protection (req-sc-002). Untouched.
-
Supply-chain threat model in Section 10 retains req-xxx mappings for all 21 enumerated attack surfaces. The new below-table note correctly traces req-tg-015 as a diagnostic-safety surface distinct from row 16 capability-scope widening, avoiding the threat-surface conflation the CEO flagged in F5. Provenance/attestations reservation (Section 10.12) names the binding targets (in-toto/SLSA, sigstore, lockfile attestations field) and remains intact.
-
Token redaction (req-sc-007) continues to cover diagnostics, logs, error messages, packed bundles, lockfiles, and persisted audit records with source-descriptor-only identification. The req-tg-015 value-exclusion clauses (both paragraph 1 non-string path and paragraph 2 dropped-field path) now explicitly extend this redaction principle to the Codex metadata diagnostic surface, closing the normative gap identified in round 1.
spec-pkgmgr-editor -- shocked_meter 8/10, confidence high
All round-1 findings closed: TOML field ordering explicitly deferred in editorial note; reserved-slot prohibition rejected with sound rationale. Three synthesis folds (F1 conforming-consumer convention, F2 undefined-accepted removal, F3 non-string value-exclusion clause) applied correctly. F4/F5 non-application is a net improvement: opaque-values statement replaces unverifiable companion cross-reference, and standalone Section 10.11 paragraph replaces imprecise row-16 graft. Zero new blocking, recommended, or nit findings. No regressions in any previously praised strength.
Both recommended findings addressed: pkg-rec-r1-1 (TOML field ordering for deployed-file hash convergence) cleanly deferred via explicit editorial-note statement in req-tg-015 ('This clause does not prescribe TOML field ordering'); pkg-rec-r1-2 (defensive reserved-slot for additional Codex-native keys) rejected with acceptable rationale -- the declarative non-extension sentence ('This clause does not require or imply preservation of any other config.toml-native key') is sufficient without converting to a MUST-NOT prohibition that would change character from 'we do not mandate X' to 'you MUST NOT do X'. 2/2 closed.
No new findings.
Preserved strengths confirmed
-
Semver dialect pinning to node-semver plus semver 2.0.0 Section 11 remains complete and unambiguous in Section 7.3.1
-
Lockfile determinism story (req-lk-005 semantic equivalence, req-lk-012 canonical content hashing, req-lk-015 tree_sha256 verification) is intact and unmodified
-
Transitive conflict policy (req-rs-001 tri-modal with defensive req-rs-013 nest-mode reservation in v0.1) is intact
-
Reserved-slot pattern well-established across Sections 4.8 (workspaces), 7.9 (version withdrawal), 10.12 (publisher attestations), Appendix B (registry HTTP API)
-
Producer/Consumer/Registry/Governance conformance classes cleanly separated in Section 11.1; req-tg-015 correctly conditional on consumer opt-in capability with 'A conforming consumer' convention restored
-
Pack/publish integrity chain (req-lk-013 archive hash, req-rs-009 mirror tolerance, req-rg-001 registry trust anchor) is intact
-
Statement count 126 (121 MUST, 5 SHOULD) consistent across Section 1.3, Appendix C footer, and Appendix D row 0.1.43
-
Section 10.11 diagnostic-safety tracing via standalone paragraph is cleaner than the original F5 row-16 graft and correctly distinguishes metadata-loss diagnostics from capability-scope widening
spec-tag-architect -- shocked_meter 8/10, confidence high
All four round-1 recommended findings are closed or cleanly deferred. The drafter made architecturally superior choices to the original synthesis in two cases: grounding field provenance without referencing an unpublished companion (tag-rec-r1-1), and placing a standalone Section 10.11 note rather than conflating threat rows (tag-rec-r1-3). No new findings, no regressions, no blocking issues. The spec remains architecturally sound and self-contained at v0.1 maturity.
4/4 closed or cleanly deferred. tag-rec-r1-1 (field-name provenance): closed -- editorial note now reads 'native Codex agent settings; their string values are opaque to this specification', establishing provenance and a clean interface boundary without fabricating a cross-reference to the unpublished Target Registry companion; architecturally superior to the synthesis F4 proposal. tag-rec-r1-2 (undefined 'accepted'): closed -- zero matches for 'accepted.*Codex' across spec body, Appendix D, and requirements manifest; gating phrase now matches the conditional-capability pattern of other opt-in requirements. tag-rec-r1-3 (diagnostic-safety asymmetry): closed -- standalone note after Section 10.11 table correctly traces the diagnostic-safety surface and explicitly distinguishes it from row 16's capability-scope widening threat; editorial note closing sentence acknowledges scoped nature without empty generalization promises; architecturally superior to the synthesis F5 proposal which would have conflated threat rows. tag-rec-r1-4 (per-target accretion): deferred_clean -- finding explicitly stated no text change needed; synthesis correctly rejected as having no actionable fold.
No new findings.
Preserved strengths confirmed
-
Extension model (req-ext-001, req-ext-002) remains sound: the x-* vendor namespace with round-trip guarantees, collision prevention, and registration discipline is intact and untouched by this amendment.
-
Amendment process discipline is exemplary: the Appendix D row 0.1.43 now reflects the round-2 folds (conforming consumer, removed 'accepted', non-string value-exclusion clause) and continues to correctly classify under Section 9.2 as a new opt-in capability with Section 9.3 pending status.
-
Forward-compatibility apparatus (Section 9 versioning, 90-day migration windows, errata vs revision distinction) remains fully intact.
-
Machine-readable contract surface: Appendix C is properly updated, all four count sites (Section 1.3, Section 8.7 trailer, Section 11.3.2 enumeration, Appendix C total) reconcile to 126 (121 MUST, 5 SHOULD). The requirements manifest YAML matches the normative text and reflects all round-2 folds.
-
Conformance class architecture (Section 11) is coherent: req-tg-015 uses the 'A conforming consumer implementation' convention (88 instances, zero bare 'A consumer implementation' in the spec), is correctly enumerated in Section 11.3.2, Section 8.7, and Appendix C, and the consumer conformance class boundary is well-defined.
-
Layering coherence improved: the editorial note grounds field provenance as 'native Codex agent settings' with 'opaque to this specification', establishing a clean interface boundary without fabricating cross-references to unpublished companion documents. The standalone Section 10.11 note correctly distinguishes the diagnostic-safety surface from the capability-scope threat in row 16.
This panel is advisory. It does not block merge.
Generated by apm-spec-guardian. This comment is AI-generated and may contain errors.
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Delta closes original alias-consistency nit; singular/plural grammar is inline-proportionate and tested; embedded-newlines TOML roundtrip verified safe. No new architectural concerns. Ship. |
| Test Coverage | 0 | 0 | 0 | Delta behavioral changes (singular grammar, embedded-newlines roundtrip, non-string value exclusion spec clause) all have multi-tier coverage with 5/5 mutation kills; no new gaps. |
| Doc Writer | 0 | 0 | 0 | The prior release-status finding is resolved; folded warning guidance and proposed Codex requirements match the implementation. No substantive delta findings. |
| DevX UX Expert | 0 | 0 | 0 | Original warning-terminology nit folded; singular grammar fix is a genuine UX improvement; no new issues. Ship. |
| CLI Logging Expert | 0 | 0 | 0 | Original nit (singular grammar in dropped-fields diagnostic) fully folded; five singular/plural variables branch correctly on len(dropped_field_names)==1; unit and integration tests assert the singular path; mutation-singular-grammar.log confirms both tests fail when singular guard is reverted on d298a41 working copy. 402 passed at d298a41. No new CLI logging concerns in the delta. |
| Supply Chain Security | 0 | 0 | 0 | Delta clean. Prior nit (embedded-newline TOML injection test) folded and verified: toml library escapes \n and " in string values, tomllib round-trip confirms no key injection. Non-string value privacy clause added to spec ('MUST NOT include the rejected value'); code already complies -- non-string diagnostic at line 536 references only field name, never fm[field]. Singular grammar change is wording-only, does not alter safety properties (printable-ASCII sanitization, count bound, length bound all preserved). Lockfile unchanged. 402 tests pass, 5 mutants killed. No diagnostic regressions, no new supply-chain attack surface. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
All six delta reviewers converge at zero findings after d298a41 folded every actionable item from the full round. The four-panel spec review lands at fold_and_ship with an empty fold queue, unanimous 8.0 shocked-meter, and zero new findings across all severity tiers. The validation suite (402 passed, 5 mutation kills, complete lint mirror, architecture boundary, conformance generation) is thorough and exact-head verified. No in-scope engineering work remains. The PR body candidate correction (scenario tiers, schema row, multiline case, and prior-run corrections) is pending publication via the guarded tool but its content is confirmed accurate; it does not gate engineering readiness. Missing historical CodeQL API-upload analysis, CODEOWNER/last-push approvals, and the protected merge queue are separate requirements outside this engineering assessment; mergeStateStatus remains BLOCKED and no bypass is authorized.
Full per-persona findings
Python Architect
No findings.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security
No findings.
Doc Writer
No findings.
Test Coverage
No findings.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Delivery and verification receipt
- Head:
d298a419a1acffde8104802432e0936c71c1a160; main:18c4c43c924ceae890fe0f2038806690e5b2d6c8. One normal fast-forward push; no force push or policy changes. - Folded: release wording, singular diagnostics, newline TOML regression, resource terminology, constant alias, and bounded req-tg-015 conformance/privacy clarifications. Scenario evidence now includes repository-defined component/e2e tiers, schema-repair and newline cases.
- Post-synthesis fulfillment: the guarded PR-body update has now succeeded and was read back. The CEO's verbatim prose above accurately described that publication step as pending when it synthesized; it is completed now. Source head and evidence are unchanged.
- Exact-head proof: 402 passed, 2 skipped; five production mutants killed, source restored byte-for-byte; complete lint mirror, architecture boundary, quality/assertions/duplicates and clean conformance generation passed. The unchanged owner gate verified its full terminal-evidence branch with zero canonical owner touches. The internal candidate was a verifier probe, not a provider-ready claim.
- Ordinary CI: 19 SUCCESS, 2 NEUTRAL, 1 legitimately SKIPPED;
gh pr checks 3150 --repo microsoft/apm --watchexited 0. Main CI, CodeQL analyses, docs, conformance. - Provenance qualifications: spec round 1 reviewed initial
24f18c7d; round 2 reviewed finald298a419. The CEO headline'sfold_and_shipnames the genuine spec synthesizer decision, not four identical reviewer verdicts; the pkgmgr dissent remains explicitly preserved above. The raw spec-synthesis '401 cases' typo remains preserved and corrected against actual 402-pass JUnit. No frozen-binary or native Codex inference claim is made. - Copilot finding 4166618929 was LEGIT and already fixed by earlier count/key-length caps; the thread remains resolved and fresh mutations prove the boundaries. Two classification rounds found no new item.
- No in-scope findings or deferrals remain. Missing historical CodeQL API-upload analysis, CODEOWNER/last-push approval and merge queue remain external requirements, not suppressed failures or granted permission.
| PR | Head | Mergeable | Provider merge state | Ordinary CI | Engineering stance |
|---|---|---|---|---|---|
| #3150 | d298a419 |
MERGEABLE | BLOCKED | green | ship_now |
Post-publication CI snapshot: 20 SUCCESS, 2 NEUTRAL, 1 legitimately SKIPPED. Updating the PR body added one successful eligibility-publication job. The CEO and body counts above record the earlier all-green snapshot of the same head; no ordinary check is pending or failed.
Preserved history
This is the new terminal advisory for the separately authorized October 6 run, based on complete actual specialist originals and genuine synthesis. Previous corrected/general full advisory and historical failed spec receipt remain unchanged history; none is retroactively declared successful. Fresh complete spec advisory records the independent spec convergence. No merge, bypass, approval, reviewer/label modification or scan restoration occurred.
Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.

Description
Important
Fresh engineering result at
d298a419a1acffde8104802432e0936c71c1a160: genuine final delta CEOship_now; ordinary CI green (19 SUCCESS, 2 NEUTRAL, 1 legitimately SKIPPED). No in-scope engineering findings remain. Provider merge state is stillBLOCKED: historical CodeQL API-upload analysis, CODEOWNER/last-push approvals and the protected merge queue remain separate requirements. None was waived; no merge or bypass is authorized.fix(codex): preserve native agent model settings
TL;DR
Codex agent conversion now preserves supplied string
modelandmodel_reasoning_effortvalues as top-level TOML keys instead of silently dropping them. Omitted fields remain absent, and untranslated metadata or non-string settings receive default-visible, actionable diagnostics. Other targets and the existing tool-restriction warning are unchanged.Note
This is bounded native-format compatibility, not model execution, cross-provider model translation, or a new
codex:namespace.Problem (WHY)
mainat7dfc5dd7: an agent declaring both settings produced onlyname,description, anddeveloper_instructions, with an empty diagnostic collector.modelormodel_reasoning_effort, the value in the file takes precedence." The generated output discarded precisely those authored overrides.model_verbosityalso disappeared without notice. Tests now inspect actual generated TOML and CLI diagnostics, following the concrete-input/output emphasis in Agent Skills: "Input/output formats".Approach (WHAT)
developer_instructions, and preserve the existingtoolswarning and every other target's renderer.Implementation (HOW)
src/apm_cli/integration/agent_integrator.py_write_codex_agent; route field-loss diagnostics through the existing collector, with ASCII-safe names and no metadata values in diagnostics.tests/unit/integration/test_agent_integrator.pytests/integration/test_codex_agent_tool_scope_contract.pydocs/src/content/docs/producer/author-primitives/instructions-and-agents.mdpackages/apm-guide/.apm/skills/apm-usage/package-authoring.mdCHANGELOG.mddocs/src/content/docs/specs/openapm-v0.1.md,docs/public/specs/manifests/openapm-v0.1.requirements.yml,tests/spec_conformance/test_manifest_reqs.pyCONFORMANCE.json,CONFORMANCE.md.apm/skills/apm-spec-guardian/assets/synthesizer-return-schema.json, generated.agents/copy,apm.lock.yaml,tests/unit/test_apm_spec_guardian_synthesizer_schema.py,tests/fixtures/spec-guardian/pr3150-synthesizer-return-original.jsondocs/src/content/docs/contributing/development-guide.mdArchitecture:
ordinary-fix._write_codex_agentremains the single Codex agent-format renderer behindintegrate_agents_for_target. No new owner, centralization, registry, routing path, or static rule is introduced.Diagram
Dashed boxes show the added field-preservation and loss-detection branches; the existing writer and diagnostic collector retain their responsibilities.
flowchart LR subgraph Source["Source"] A[".agent.md frontmatter and body"] end subgraph Render["AgentIntegrator._write_codex_agent"] B["name, description, Markdown body"] C["Supplied string model and model_reasoning_effort"] D["Dropped fields and non-string settings"] end subgraph Output["Output"] E["write_text_lf: .codex/agents/name.toml"] F["DiagnosticCollector.lossy_agent_compilation"] end A --> B A --> C A --> D B --> E C --> E D --> F classDef new stroke-dasharray: 5 5; class C,D new;Trade-offs
codex:blocks untranslated rather than expanding the accepted scope. Their loss is explicit.Benefits
Issue and approved scope
Issue: #3126
Human scope-approval comment (sole nominated record as of this update): #3126 (comment)
The approved scope preserves the two documented native fields (
model,model_reasoning_effort), diagnoses unsupported loss explicitly, and retains other targets unchanged. The separately authorized October 6 run used six genuine complete-input general reviewers and four full-source spec reviewers, then folded their accepted findings ind298a419. All six genuine delta reviewers report no remaining findings. The new complete spec advisory recordsfold_and_ship. The genuine final general CEO received complete unabridged full/delta originals, spec returns, conversation and execution evidence, and independently returnedship_nowwith no follow-ups.Preserved previous-run corrections and provider history (not current-head acceptance)
The earlier
24f18c7drun independently verified complete-source spec synthesis and architect/coverage/docs narrow review of4b7e4118..24f18c7d. Its corrected general recommendation wasship_with_followups, not an accepted terminalship_now. Faithful abbreviated reviewer inputs were not verbatim full originals and are not retroactively represented as such.That run's no-bypass provider read reported head
24f18c7d3f7be1269381fe15a936521fb1866c65asMERGEABLE, protectedBLOCKED, and merge requirementsUNMERGEABLEagainst main18c4c43c924ceae890fe0f2038806690e5b2d6c8. Its failed Repo-rules condition explicitly said: "Code scanning is still expecting 1 result from CodeQL for 83e9d6b or 24f18c7." This is a separatecode_scanningrule, not a required status-check context named CodeQL; the ordinary required contextgatepassed. The missing historicalAPI upload / <default>analysis is not evidence of a new vulnerability. Standard analysis-job success and the neutral summary do not satisfy that missing analysis. Restoration was blocked by package-read authorization; offline database preparation is not a completed scan or upload. This run did not attempt restoration or claim it succeeded.CODEOWNER review, last-push approval and the protected SQUASH merge queue remain separate requirements. The requested reviewer
sergio-sisternes-epamis untouched. The extra-unattributed-approval flag has no effect with the ruleset's zero numeric approval count and is not an additional current gate. No merge, auto-merge, approval bypass or scanning-rule waiver is authorized. The premature terminal advisory was corrected in place; previous failed receipts remain historical. The October 6 engineering result does not reopen or reclassify those exhausted attempts.Fixes #3126
Type of change
Testing
The full local test suite was not rerun. The exact-head focused/schema/conformance suite, quality gates, complete lint mirror and all ordinary provider CI checks pass; the unchecked blanket claim above is intentionally narrower than saying every repository test ran locally.
Validation evidence
Current exact-head evidence uses
UV_FROZEN=true, the worktree's installed source entrypointAPM_BINARY_PATH="$PWD/.venv/bin/apm", and an evidence-localTMPDIR. This is not frozen-binary or native Codex-inference testing. Five production mutants (emission, singular grammar, count cap, length cap, key sanitization) were killed, then the source was restored byte-for-byte. Deterministic owner detection found zero canonical owner touches; the unchanged completion verifier exercised its full terminal-evidence branch (terminal_evidence_required: true), not its blocked-status shortcut.Ordinary CI at
d298a419: 19 SUCCESS, 2 NEUTRAL, 1 legitimately SKIPPED;gh pr checks 3150 --repo microsoft/apm --watchexited 0. Main CI, CodeQL analyses, docs build, spec conformance. Both ordinary CodeQL analyses succeeded; that does not establish restoration of historical API-upload analysis.Current exact-head execution, d298a41
uv run --frozen --extra dev pytest -q tests/unit/integration/test_agent_integrator.py tests/integration/test_codex_agent_tool_scope_contract.py tests/unit/test_apm_spec_guardian_synthesizer_schema.py tests/spec_conformance/ --basetemp=/Users/danielmeppiel/.copilot/session-state/cf24cbf0-c318-4853-a131-4be723d1cea8/files/pr-3150-evidence/pytest-final --junitxml=/Users/danielmeppiel/.copilot/session-state/cf24cbf0-c318-4853-a131-4be723d1cea8/files/pr-3150-evidence/functional-final.xmluv run --extra dev pytest -p no:cacheprovider -q tests/qualityuv run --frozen python scripts/check_test_assertions.pyuv run --frozen python scripts/check_exact_test_duplicates.pyuv run --extra dev ruff check src/ tests/ scripts/lint_architecture_boundaries.py scripts/architecture_linter/uv run --extra dev ruff format --check src/ tests/ scripts/lint_architecture_boundaries.py scripts/architecture_linter/uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ scripts/lint_architecture_boundaries.py scripts/architecture_linter/bash scripts/lint-auth-signals.shbash scripts/lint-architecture-boundaries.shuv run --frozen --extra dev python -m tests.spec_conformance.orphan_checkuv run --frozen --extra dev python -m tests.spec_conformance.gen_statementgit diff --exit-code CONFORMANCE.json CONFORMANCE.mdThe YAML I/O, 2100-line and portable-path CI guard equivalents also pass.
uv.lockand the APM lock were unchanged in this fold. The initial focused run exposed one stale spec-text assertion (1 failed, 401 passed, 2 skipped); it was corrected and the complete command above reran successfully. The genuine final spec synthesis's "401 cases" phrase is a retained reporting typo; the actual JUnit/log result is 402 passed, 2 skipped. Mermaid validation: installedmmdc, exit 0.Preserved historical validation and corrections (not current-head acceptance)
The closed schema/report unit on 2026-10-05 at
4b7e4118bb4250691fdcdfa6db44ce24dbdffc3dhad 304 focused/schema-conformance passes and 2 skips. It changed no production/spec source oruv.lock; only the two authorized generated-schema integrity hashes changed in the parsed APM lock. Those results do not establish compliance with every promised pre-push constraint.24f18c7duv run --extra dev pytest tests/unit/integration/test_agent_integrator.py tests/integration/test_codex_agent_tool_scope_contract.py -q4b7e4118uv run --frozen --extra dev python -m pytest -q tests/unit/test_apm_spec_guardian_synthesizer_schema.py tests/spec_conformance/4b7e4118uv run --frozen --extra dev python -m tests.spec_conformance.orphan_check4b7e4118bash tests/spec_conformance/mode_b_detector.sh4b7e4118uv run --frozen --extra dev python -m tests.spec_conformance.gen_statementandgit diff --exit-code CONFORMANCE.json CONFORMANCE.mdEarlier
7562a0c9attempts reported 2164 affected tests, 92 agent/CLI tests after merging main, 64 quality tests and 14 link tests. Their detailed commands remain in the preserved GitHub body edit history. The closed schema/report unit's pre-push provider snapshot preceded its push and is not all-green final-head evidence. The historical before-fix run recorded 15 failed, 6 passed, 69 deselected; the earlier emission mutation recorded 5 failed, 1 passed, 86 deselected. Those are historical fail-before claims, distinct from this run's five executed mutants. A prior shell guard attempt lackedrgand was discarded; only the successful guard equivalents counted.Scenario Evidence
tests/integration/test_codex_agent_tool_scope_contract.py::test_codex_agent_native_models_and_dropped_metadata_reach_cli_output(regression-trap for #3126)tests/unit/integration/test_agent_integrator.py::TestCodexAgentIntegration::test_codex_native_settings_reach_generated_agenttests/unit/integration/test_agent_integrator.py::TestCodexAgentIntegration::test_codex_non_string_settings_are_diagnosed;test_codex_unsupported_metadata_is_diagnosed_without_passthroughin the same classtests/unit/integration/test_agent_integrator.py::TestCodexAgentIntegration::test_codex_metadata_change_preserves_other_targets;tests/integration/test_codex_agent_tool_scope_contract.py::test_codex_agent_tool_scope_is_never_silently_losttests/unit/test_apm_spec_guardian_synthesizer_schema.py::test_valid_full_report_and_original_return_fail_broken_schema_pass_fixed;test_defer_v0_2_missing_reserved_slot_anchor_rejectedtests/unit/integration/test_agent_integrator.py::TestCodexAgentIntegration::test_codex_native_settings_reach_generated_agent[embedded-newlines-are-values]Behavioral tiers follow
.github/instructions/tests.instructions.md: filesystem/in-process tests are component; installed-source CLI subprocess cases are e2e. The schema regression module contains 12 collected cases.How to test
apm install --target codex, and inspect its.tomlfile: both settings should be top-level keys.model_verbosity: low; installation should name that dropped field and explain that APM does not translate it.Spec conformance (OpenAPM v0.1)
If this PR changes behaviour that an OpenAPM v0.1
req-XXXcovers,confirm the three-step ritual in the
development guide:
docs/src/content/docs/specs/openapm-v0.1.mdupdated(new/changed
<a id="req-XXX"></a>anchor + prose + Appendix Crow).
docs/src/content/docs/specs/manifests/openapm-v0.1.requirements.ymlupdated.
@pytest.mark.req("req-XXX")test undertests/spec_conformance/added or extended.CONFORMANCE.{md,json}regenerated viauv run --extra dev python -m tests.spec_conformance.gen_statementand committed.
The earlier spec unit added proposed
req-tg-015in Section 8.5.1, its manifest entry and real Codex/non-Codex behavioral coverage. The actual committed manifest isdocs/public/specs/manifests/openapm-v0.1.requirements.yml; the template path above is retained verbatim.req-tg-006is unchanged.The closed schema/report unit ending at
4b7e4118bbchanged no normative spec or production code: it repaired the review schema, added 12 focused regression cases and a retained historical fixture, updated the two generated-schema lock hashes, and added a non-normative development-guide clarification. Exactly one genuine targeted synthesizer ran in that closed unit; its custom fragment recommended patch-queue F9 treatment, but the approved full-return contract was not delivered. The worker-assembled full report also retained conflicting F9 prose. See the existing historical spec advisory for attribution and that blocked disposition.The earlier separately authorized renewed run added two req-tg-015 editorial clarifications and folded source/test recommendations. Its subsequent genuine full-schema spec-editor-synthesizer return used the complete repaired schema, four retained genuine panelist returns and original synthesis; exact inputs and output were independently verified. That closed the full-return component in that renewed run, not the historical failed attempt or the outstanding merge gates. No broader applicability-field feature decision is made here.
The October 6 run subsequently executed two actual full-source spec-panel rounds and a genuine complete-schema final synthesis at
d298a419, with no remaining fold/defer queue. It clarifies conforming-consumer scope, opaque native values and non-string diagnostic value exclusion without a broader applicability feature. Checks 1-10 pass; checklist 11 notes Python changes covered by the actual general panel and full lint. Existing historical comment 5972650749 remains unchanged.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com