test: Add 25 WRAP_ACTIONS conformance fixtures for coverage gaps - #157
test: Add 25 WRAP_ACTIONS conformance fixtures for coverage gaps#157leongdl wants to merge 1 commit into
Conversation
|
CI note: two fixtures fail on CI but passed local validation against openjd-rs main (wrap-failed-exit-hook-own-onexit-still-runs, wrap-grand-child-fails-under-enter-hook) — likely a released-vs-main CLI version difference or Linux/macOS platform difference; investigating before marking ready. Mainline CI is also currently red for every PR due to two pre-existing broken fixtures unrelated to this change. |
There was a problem hiding this comment.
Quorum finding: NOT-GOOD as-is — the added tree diagram documents proposed/ containing wrap-repr-py-escapes-newline-in-wrapped-args.test.yaml and a proposed/README.md, but neither exists on this branch (they live on PR #162's branch). Either note the dependency on #162 or drop those lines; the 24 real fixture entries in the README are complete and correctly grouped.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: §4.3.1 / RFC 0008 scope rule (WrappedAction.* only inside the three wrap hooks). Single defect (the onEnter funcall); the in-hook repr_py uses are legal, so a correct implementation can only reject via the scope rule. Targets validator expression-walking blind spots untested on mainline.
There was a problem hiding this comment.
Quorum verdict: GOOD-WITH-NITS (3 reviewers flagged the same nit) — spec: §4.3.1 scope rule + §3.6.2 let-scope table. Fixture is invalid under any reading, but the header's "single defect = onEnter's use of the binding; the hooks' use is fine" framing conflicts with §3.6.2: WrappedAction.* is not in EnvironmentScript.let's symbol table at all, so the let binding itself is the violation. Reword the comment; the fixture can't distinguish let-walking validators from generic let-scope checks.
There was a problem hiding this comment.
Quorum verdict: GOOD-WITH-NITS — spec: §4.3.1 (WrappedEnv.* only in env hooks). Clean single defect. Nit: repr_py is gratuitous (bare {{WrappedEnv.Name}} suffices) and adds a wrong-reason surface — an implementation lacking repr_py rejects for an unrelated reason and still passes this .invalid test. Mitigated suite-wide by valid fixtures using repr_py.
There was a problem hiding this comment.
Quorum verdict: GOOD-WITH-NITS — spec: §4.3.1 (WrappedStep.* only in onWrapTaskRun). Single defect in the enter hook. Same gratuitous-repr_py nit as siblings.
There was a problem hiding this comment.
Quorum verdict: GOOD-WITH-NITS — spec: §4.3.1. Correct twin of the enter-hook fixture; coverage reviewer notes it's a near-duplicate (same validator scope-table path) — cheap enough to keep, but the pair likely catches the same bugs. Same repr_py nit.
There was a problem hiding this comment.
Quorum verdict: GOOD-WITH-NITS — spec: §5 (timeout fmtstring @fb1) + §5.3 (mode/notifyPeriod fmtstring @fb1), accept-side twin of two mainline reject fixtures — a real gap filled. Nit: it sits in WRAP_ACTIONS/env_templates but additionally requires FEATURE_BUNDLE_1, and the runner does not gate by declared extensions — an implementation with WRAP_ACTIONS+EXPR but no FB1 falsely fails this "valid" fixture. Consider noting the extra requirement or relocating.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: §4.3 constraint 1 (all-or-nothing). Minimal, schema-valid but for the missing onWrapTaskRun; rejection can only come from the named rule. Completes the invalid matrix with its three siblings (mainline had only 2 of 6 cells).
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: §4.3 constraint 1 (all-or-nothing), enter-only subset. Clean single defect.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: §4.3 constraint 1, exit-only subset. Coverage reviewer rates it the weakest matrix cell (siblings almost certainly catch the same validators) but completing the matrix is cheap and guards asymmetric validators.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: §4.3 constraint 1, missing-enter direction. All refs in legal hooks; only the missing onWrapEnvEnter can trigger rejection in a correct implementation.
There was a problem hiding this comment.
Quorum verdict: GOOD-WITH-NITS — spec: RFC 0008 stdout forwarding (runtime scans the wrap script's stdout only, never the grand-child's stream directly). Effect-based negative control, correctly paired with the excludes-host-vars sibling. Nit (service-compat reviewer): the forbidden literal LOST_VAR=should-not-surface also occurs inside the resolved hook command line via repr_py(WrappedAction.Args); a runner that echoes resolved commands into the session log false-fails. Dropping that forbidden line (keeping the WAENV=LOST_VAR one) makes it robust.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: §4.3.1 / RFC 0008 (WrappedAction.Environment carries only session-defined variables). Positives (STATIC_VAR, DYNAMIC_VAR) prevent a vacuous pass via empty forwarding; forbidden PATH/Path genuinely discriminates os.environ-merging runtimes. First host-var-exclusion fixture in the suite.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: RFC 0008 failure semantics + cleanup guarantee (wrapping env's own onExit still runs after a failed onWrapEnvExit). WRAPENV_OWN_EXIT_RAN fails any teardown-aborting runner; a runtime ignoring the hook failure exits 0 and fails taskFailure. Omitting exitCode is honestly justified (teardown failures bypass the runner's exit-code regex).
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5, one soft nit) — spec: RFC 0008 failure semantics at env-enter position + cleanup row. exitCode 9 is the only non-zero exit line, forbidden TASK_BODY_RAN pins the task never started, WRAP_EXIT + own-exit markers pin cleanup. Nit: exitCode-at-enter-phase reporting is a runner-shaped channel; acceptable.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: RFC 0008 wrap ordering across scope tiers (job-level wrapper intercepts a step-level env). WENTER/WEXIT=InnerStepEnv can only come from cross-scope interception; forbidden STEP_ENV_*_BODY fails a same-tier-only runtime. First cross-scope fixture in the suite.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — baseline control: exercises no WRAP_ACTIONS text itself; its role is to give the sibling fixtures' forbidden-body assertions meaning (a runtime that suppresses inner bodies even unwrapped fails here). Correct as a paired negative control.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: RFC 0008 macro scanning of the wrap script's own stdout. The single assertion is effect-based: WAENV=WRAP_SELF_VAR can only appear if the runtime scanned the hook's stdout and fed it into WrappedAction.Environment. Discriminates hook-skipping and grand-child-only-scanning runtimes.
There was a problem hiding this comment.
Quorum verdict: GOOD-WITH-NITS — spec: RFC 0008 macro propagation during teardown + LIFO exit order, validly encoded via causality (EXIT_VAR in Alpha's dump requires Beta exited first) despite the runner's unordered matching. Nit: the forbidden WAENV[EnvBeta]=EXIT_VAR line is vacuous — dump-before-subprocess is fixture-controlled and args are interpolated at launch, so no runtime behavior can trigger it; the comment overstates what it pins.
There was a problem hiding this comment.
Quorum verdict: GOOD-WITH-NITS (2 reviewers same nit) — spec: RFC 0008 (WrappedAction.Environment accumulates openjd_env from earlier wrapped task actions). Discriminates per-step-rebuild and env-lifecycle-only harvesting. Nit: correctness assumes tasks run in ascending parameter order, which neither spec nor runner asserts — a conformant scheduler running tasks reversed would falsely fail. Consider making the second task's marker order-independent or documenting the assumption.
There was a problem hiding this comment.
Quorum verdict: GOOD-WITH-NITS (3 reviewers same concern) — spec: RFC 0008 macro forwarding. The assertions are log-text presence only: they pass whether or not the runtime recognizes openjd_progress/openjd_status, and the forwarding is fixture code — so what's pinned is "hook ran + macro lines not stripped from the log". A runtime that consumes recognized macro lines from its log is arguably conformant yet fails. The header's honest scope note acknowledges this; consider downgrading the fixture's claim or asserting an actual progress/status effect if the harness ever exposes one.
There was a problem hiding this comment.
Quorum verdict: GOOD (4/5) with one concrete dissent worth fixing — spec: RFC 0008 (wrap scripts forward wrapped-process stderr). Dissent (adversarial reviewer): a runtime that accepts WRAP_ACTIONS but never fires the hooks runs onRun directly and still emits both expected lines (STDERR_MARKER, STDOUT_BODY_RAN) — nothing asserts the wrapped path was taken, and the forwarding itself is fixture code. Cheap fix: print a WTASK sentinel from onWrapTaskRun and add it to expected.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: RFC 0008 (every inner env's lifecycle intercepted; WrappedEnv.Name fresh per invocation). A name-caching runtime misses WENTER=EnvBeta; forbidden bodies pin replacement; the no-wrap control gives the forbiddens meaning. Comment honestly defers LIFO to the exit-hook fixture.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: RFC 0008 / §4.3.1 (onWrapTaskRun fires per task with fresh WrappedStep.Name). Stale-name and stop-wrapping-after-first-step runtimes both miss WSTEP=StepTwo; forwarded body lines pin the grand-children ran under the hook.
There was a problem hiding this comment.
Quorum verdict: GOOD-WITH-NITS — spec: RFC 0008 failure-semantics parity (unwrapped baseline, exit 42). Sound as the unwrapped half of a parity pair with wrap-exit-status-python; exitCode 42 is the only non-zero line so the runner's first-match rule is safe. Nits: exercises no WRAP_ACTIONS text on its own, and nothing mechanically ties the two exit-42 codes together (comment-only coupling); placement in WRAP_ACTIONS/ is arguable.
There was a problem hiding this comment.
Quorum verdict: GOOD (5/5) — spec: §4.3 constraint 5 (nothing-to-replace: variables-only env fires no env hooks). Forbidden WENTER/WEXIT=VarsOnlyEnv catches per-transition hook firing; expected WTASK_RAN prevents a pass via no wrapping at all; WAENV=STATIC_FROM_VARS pins the variables map was still applied. Well-balanced positive/negative — and directly protects the service's variables-carrying queue environments.
|
Quorum review (5 independent agents: spec-literalist, adversarial, test-craft, service-compat, coverage). 24 fixtures reviewed; per-fixture verdicts posted as file comments. Net: 15 GOOD, 9 GOOD-WITH-NITS, plus one README defect. Strong PR: completes the all-or-nothing matrix (all 6 invalid cells), fills genuinely untested §4.3/§4.3.1 and RFC 0008 rules (host-var exclusion, cross-scope wrapping, failure semantics, exit-phase macro harvesting), and no fixture conflicts with the Deadline Cloud service's WRAP_ACTIONS usage. Actionable items: (1) README documents a proposed/ dir that lives on PR #162's branch, not this one; (2) wrap-stderr-forwarded needs a hook sentinel to avoid passing when hooks never fire; (3) a few vacuous/log-shape assertions flagged below. Still-open gaps this PR doesn't claim: repr_cmd/repr_pwsh wrap fixtures, real cancelation exercise, Windows coverage. |
25 fixtures closing gaps from the RFC 0008 coverage audit: absent-by-design host vars, macro-by-emitter matrix, stderr forwarding, variable scope negatives (incl. in-expression and in-let variants), the four missing all-or-nothing hook subsets, nesting depth 2, variables-only inner env, failure-path breadth, and the FEATURE_BUNDLE_1-gated fmtstring positive twin. Suite: 72 -> 97 passing. Spec-correct fixtures that FAIL against current implementations are split to the companion branch conformance-wrap-actions-expected-failures. Review: quorum-review fixes — WTASK_HOOK_RAN sentinel pins the wrapped path in the stderr-forwarding fixture; dropped a command-echo-prone and a vacuous forbidden line; replaced gratuitous repr_py with bare references in the three variable-scope invalids; corrected the let-scope framing per Template Schemas 3.6.2; documented the task-order assumption, the FEATURE_BUNDLE_1 requirement of the fmtstring accept fixture, and the macro-consuming-runtime caveat; README no longer references the expected-failures PR's proposed/ directory. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
a25378a to
e09f0fc
Compare
|
Quorum-review fixes applied and pushed (rebased onto mainline 3069673):
Verified: full WRAP_ACTIONS suite (97 fixtures incl. this PR's) passes 97/97 against openjd-rs upstream/main |
Summary
25 WRAP_ACTIONS fixtures closing gaps identified by a systematic coverage audit of RFC 0008 against the existing suite. All 25 validated individually through the conformance runner against openjd-rs; full suite goes 72 → 97 passing, 0 failing.
Areas covered
HOME,PATH,OPENJD_*) must NOT appear inWrappedAction.Environment— pinned withexpected.forbidden. Previously a runtime forwardingos.environwholesale passed every fixture.openjd_envemitted by the wrap script itself; by a task grand-child and visible in the next task'sWrappedAction.Environment; by a grand-child underonWrapEnvExit;openjd_progress/openjd_statusforwarding; stderr forwarding; and the negative control that a wrap script discarding grand-child stdout loses the macro (pins "MUST NOT scan the grand-child directly").WrappedEnv.NameinonWrapTaskRunrejected;WrappedStep.Namein both env hooks rejected; out-of-scopeWrappedAction.*buried inside an EXPR function call and inside aletbinding — a validator that only walks format strings passes the pre-existing negative and misses these.WrappedEnv.Namecorrect per invocation.variables:-only inner environment (noscriptat all) runs no wrap hooks — named in the RFC and schema constraint 5, previously untested.onWrapEnvExit→ wrapping env's ownonExitstill runs; grand-child failure under the enter hook propagates; unwrapped-parity control.timeoutandcancelation.modeon wrap hooks accepted when the extension IS listed (only the reject side existed).Verification
Every fixture executed via
run_openjd_cli_tests.pyagainst openjd-rs. Negatives verified to reject for the exact RFC 0008 rule under test (not an incidental reason). Cross-platform viapython -c; no newrunOngates.Companion PR
Spec-correct fixtures that FAIL against current implementations are in a separate expected-failures PR (branch
conformance-wrap-actions-expected-failures) so this one merges green.