[codex] Add validation IDs for multi-finalize grading#5570
Merged
Conversation
…tion-id # Conflicts: # package.json
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
There was a problem hiding this comment.
Test-tooling and compliance-scenario change — stable validation IDs plus tightened atomic-success assertions on the refine finalize-exclusivity storyboard. The right shape: it makes multi-finalize grading failures name the exact assertion that failed, and it brings the #4107 doc bullet back in line with the schema instead of letting it drift.
Things I checked
- Not spec drift — drift removal. ad-tech-protocol-expert: sound. The old whats-new-in-3-1.mdx bullet ("multi-finalize is rejected") was the stale text. static/schemas/source/media-buy/get-products-request.json:55 already encodes the broadened rule (atomic multi-finalize allowed; reject with MULTI_FINALIZE_UNSUPPORTED preferred / INVALID_REQUEST otherwise), landed upstream in #4796 (4c12454). The rewritten bullet at docs/reference/whats-new-in-3-1.mdx:33 is a faithful paraphrase.
- MULTI_FINALIZE_UNSUPPORTED is a registered error code, not invented for the scenario — static/schemas/source/enums/error-code.json:59 (value), :153 (description), with a 3.1 disposition in scripts/error-code-drift-dispositions.json:129.
- Every asserted field exists in the response schema. refinement_applied[].proposal_id / .status (with applied) at get-products-response.json:146-174; proposals[].proposal_status resolves to enums/proposal-status.json which includes committed. The conformance test asserts real fields.
- Validation-check kinds are authored. field_value, field_contains, error_code, any_of all present in runner-output-contract.yaml, so the check-enum source lint passes.
- New lint test is wired in both places — .github/workflows/build-check.yml:60 and the test script in package.json. Won't silently fall out of CI.
- Changeset present and correctly typed patch (.changeset/multi-finalize-validation-ids.md) — matches the precedent from the scenario's origin PR #4936. No schema or wire-shape change.
- skip_if duplicate-proposal-id guard (context.proposal_id_2 == context.proposal_id_1) is a sensible defense against sellers that return the same proposal twice, and the new .cjs test enforces it across all three multi-finalize phases.
Follow-ups (non-blocking — file as issues)
- error_code check uses allowed_values: (refine_finalize_exclusivity.yaml:474). Pre-existing — this PR only added the id: to that block. The runner-output-contract documents error_code with a singular value:. Worth confirming the runner honors allowed_values on an error_code check, or that assertion is silently a no-op.
- Atomic branch assumes strict positional binding refinement_applied[0] -> proposal_id_1, [1] -> proposal_id_2. Schema-justified (the response mandates positional matching to the request refine[]), but stricter than the "atomic at the observation point" prose. Fine given the authored request order.
Minor nits (non-blocking)
- Snippet cap is no longer a hard cap. server/tests/manual/storyboard-report-format.ts:182-186 slices to maxLength and then prepends the validation ID, so output can run maxLength + len(id) + 2. The unit test asserts this overflow. Console-only, but the param name implies a bound — reserve room for the prefix or document that maxLength bounds only the detail portion.
- Double ID insertion in the non-verbose path. formatStepFailureDetail already embeds the id per validation (L162), then formatFailureDetailSnippet may prepend f.validationId again. The snippet.includes(validationId) guard (L183) only suppresses the second insert if the ID survives the 160-char truncation. Net effect is benign — you still get one correct ID up front — but a one-line comment on the interplay would stop a future reader from "simplifying" one of the two away.
- storyboard-smoke.ts:135 passes missingDescription: empty string. A failed validation with neither description nor error now renders empty, which can leave stray "; " separators in the joined summary. Not a regression (old v.error || v.description had the same gap), but the "(validation failed)" default would be strictly more informative.
LGTM. Follow-ups noted below.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds stable validation IDs to the refine finalize-exclusivity storyboard so multi-finalize failures identify the exact assertion that failed. Updates manual storyboard and Addie conformance reporting to preserve validation IDs, including non-verbose truncated output. Tightens the atomic multi-finalize branch to avoid order-coupled proposal assertions and documents the #4107 behavior. Validated with focused storyboard ID tests, formatter/Addie unit tests, compliance build/typecheck, precommit, and the pre-push storyboard/docs checks.