From 13646647690f83648aa301d685b67691cca0e647 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 18 Aug 2026 13:18:01 +0000 Subject: [PATCH] feat(resource-value-verification): verdict aggregation, reporting, cleanup (task 7.1) aggregate_verdict()/build_verdict_record() combine the three per-layer check results into a single per-resource comparison, self-contained and invokable with synthetic records (task 8.2). Uses ASCII Unit Separator (\x1f) as the record delimiter after discovering bash's IFS tab-splitting silently collapses adjacent empty fields even with IFS set to only a tab (bug-1581), which corrupted UNREACHABLE-record field alignment. Emits one machine-parseable VERDICT line (resources/layers_checked/ mismatches/unreachable/rejected/result) plus the terminal PASS/FAIL oracle line and matching exit code. Cleanup is the same trap-based mechanism task 5.1 registered for the abort path - logs always preserved regardless of --keep-running. Round-2 fix: main()'s live-data wiring (build_verdict_records_from_tsv()) derived a layer's status from array-key existence rather than the actual recorded outcome - since tasks 6.3/6.4 always set the key (even on a genuine timeout, just with an empty value), every real UNREACHABLE was silently misclassified as MISMATCH. Fixed to check SCHEDULER_CHECK_RESULTS/ APISERVER_CHECK_RESULTS string content instead (bug-1582). Live-confirmed by killing the scheduler mid-run: VERDICT now correctly reports unreachable=9 mismatches=0, not the reverse. SKILL.md rewritten to describe the now-complete, functional tool. Co-Authored-By: Claude Sonnet 5 --- .claude/skills/xce-test-resources/SKILL.md | 330 ++++++++------ .../xce-test-resources/run-resource-verify.sh | 419 ++++++++++++++++-- .../resource-value-verification/tasks.md | 3 +- 3 files changed, 579 insertions(+), 173 deletions(-) diff --git a/.claude/skills/xce-test-resources/SKILL.md b/.claude/skills/xce-test-resources/SKILL.md index 035c0680..3f448cbe 100644 --- a/.claude/skills/xce-test-resources/SKILL.md +++ b/.claude/skills/xce-test-resources/SKILL.md @@ -6,80 +6,70 @@ model: haiku # XCE Resource-Value Verification -## Status: deployment step functional; injection/verification not yet built +## Status: complete and functional (tasks 1.1-7.1 all done) -`run-resource-verify.sh` now exists (task 5.1) and implements the -**deployment step only**: it invokes `xce-deploy-ha`'s `deploy-ha.sh` into a -fresh scratch directory, then its generated `start-all.sh`, confirms -controller + scheduler (`-vvv`) + apiserver (debug logging) are all -genuinely up, and cleans up (trap-based, covers a mid-deployment abort, not -just a completed run — see Error handling below). Invoking -`/xce-test-resources` today runs a real deploy-confirm-cleanup cycle but -does **not yet** inject a resource value or check any layer — it exits 0 on -a successful deployment/cleanup and non-zero on a deployment failure, not on -an injection PASS/FAIL verdict. Report this gap rather than fabricate a -verdict if asked to check actual resource-value propagation. - -The rest of the orchestration logic is built incrementally by later tasks in -the same spec: -- **Task 2.1/2.2** — `resupdate` accepts any registered resource name and - reports per-name accept/reject (closes the gap this whole feature exists to - close; `resval_typeresolve.c`'s fallback path). Done. -- **Task 3.1** — `xce-deploy-ha`'s `deploy-ha.sh` grows a conditional apiserver - step + a combined `start-all.sh` (controller → scheduler → apiserver). Done. -- **Task 4.1/4.2** — one trace-log line each in the scheduler - (`core_events.c`, `XCE_CORE_EVENT_RESVALS_DELTA`) and the apiserver - (`handler.go`'s `OnResourceLocationUpdated`) — what this skill will - tail/grep for. Done. -- **Task 5.1** — wires `start-all.sh` + trap-based cleanup into this skill. - Done (this is the deployment step described above). -- **Task 6.1–6.4** — worker join + injection, and the controller/scheduler/ - apiserver checks themselves. Not yet built. -- **Task 7.1** — verdict aggregation, the VERDICT line, and cleanup-on-every-path. - Not yet built. - -Once tasks 6.1–7.1 land, this section (and the rest of this document's -hedging about the VERDICT line/PASS-FAIL oracle being aspirational) should -be removed. - -## What this skill will do (target behavior) +`run-resource-verify.sh` implements the full flow described below: deploy +controller + scheduler + apiserver, join a synthetic worker, inject one or +more resource values, verify all three layers, emit a VERDICT line plus the +standard terminal PASS/FAIL oracle line, and clean up. Invoking +`/xce-test-resources` runs the whole thing end-to-end and its exit code +reflects the real overall verdict (0 = PASS, non-zero = FAIL) -- not just +whether the deployment stood up. Bug-1569 required deploying real worker/controller/scheduler/apiserver daemons and waiting on periodic reporting cycles just to confirm a resource-value routing fix. This skill turns that multi-minute manual -investigation into one command: +investigation into one command. + +## What this skill does 1. **Deploy** controller + scheduler + apiserver into a fresh scratch directory by invoking `xce-deploy-ha`'s `deploy-ha.sh` (Step 3) and then - its generated `start-all.sh` — no manual per-component sequencing lives in - this skill; `start-all.sh` already gates scheduler/apiserver startup on - `xce-deploy-ha`'s existing controller health-check. -2. **Inject** — join a synthetic worker through `xce_test`'s worker context + its generated `start-all.sh` -- no manual per-component sequencing lives + in this skill; `start-all.sh` already gates scheduler/apiserver startup + on `xce-deploy-ha`'s existing controller health-check. A `trap`-based + cleanup is registered before the first process starts, so it fires on + every exit path -- a completed PASS/FAIL run, a mid-deployment abort, or + an external SIGINT/SIGTERM -- not just a successful completion. +2. **Inject** -- join a synthetic worker through `xce_test`'s worker context (no real worker daemon process) and run `resupdate -n -v ` - for one or more resource names. -3. **Verify all three layers**: - - **Controller** — query via `xce_test`'s client-context `resval` and - compare against the injected value. - - **Scheduler** — tail/grep the live scheduler daemon's own log file for - the trace line task 4.1 adds, with a short poll interval up to a - generous bounded wait (no record/replay/REPL). - - **Apiserver** — tail/grep the apiserver's own log file for the trace - line task 4.2 adds, same bounded-wait posture. - Each layer's observed value is reported distinctly, so a mismatch is - attributable to a specific layer, not just "something failed." -4. **Report** a single VERDICT line plus the standard terminal PASS/FAIL - oracle line and exit code. + for one or more resource names in a single RPC round trip. Every + submitted name gets a definitive "accepted" or "rejected: \" + outcome. +3. **Verify all three layers**, for every accepted name: + - **Controller** -- query via `xce_test`'s client-context `resval` and + compare against the injected value (run nested inside the still-open + injecting worker session, so host-attrs resources like `cpuf` are + never reset by a premature disconnect -- see bug-1579). + - **Scheduler** -- tail/grep the live scheduler daemon's own log file + (`resval applied host= resource= value=`, `-vvv`) with a short + poll interval up to a 15s bounded wait (no record/replay/REPL). + - **Apiserver** -- tail/grep the apiserver's own log file (zap + `"resource-location stored"` with a trailing JSON field block), same + bounded-wait posture. + Each layer's observed value is reported distinctly (MATCH / MISMATCH / + UNREACHABLE), so a failure is attributable to a specific layer, not just + "something failed" -- UNREACHABLE (bounded wait timed out, no trace line + ever appeared) is always reported distinctly from MISMATCH (a trace line + appeared with the wrong value). +4. **Aggregate and report** -- combine the injection accept/reject outcome + and all three layers' results into one machine-parseable VERDICT line + plus the standard terminal PASS/FAIL oracle line (see below). A rejected + resource name is a run-level failure in its own right, independent of + whatever the (nonexistent, since a rejected name is never actually + applied anywhere) per-layer results would say. 5. **Clean up** every process and runtime/data-directory artifact this run - created — unconditionally, on PASS, FAIL, or a mid-deployment abort alike - — unless the operator explicitly asked for the deployment to be left - running. **Log files under the scratch directory are always preserved**, - independent of that flag, so a FAIL verdict's underlying trace-log - evidence stays inspectable after the run (a deliberate divergence from - `xce-deploy-ha`'s own `cleanup.sh`, which wipes `logs/*.log`). + created -- unconditionally, on PASS, FAIL, or a mid-deployment abort + alike -- unless the operator explicitly asked for the deployment to be + left running (`--keep-running`). **Log files under the scratch directory + are always preserved**, independent of that flag, so a FAIL verdict's + underlying trace-log evidence stays inspectable after the run (a + deliberate divergence from `xce-deploy-ha`'s own `cleanup.sh`, which + wipes `logs/*.log`). -**Idempotency & recovery**: every invocation is safe to re-run — a fresh +**Idempotency & recovery**: every invocation is safe to re-run -- a fresh scratch directory per run, following `xce-deploy-ha`'s existing "every -invocation is a fresh, self-contained deploy" convention — with no shared +invocation is a fresh, self-contained deploy" convention -- with no shared mutable state carried across runs. ## Usage @@ -91,124 +81,176 @@ run-resource-verify.sh [--keep-running] [--dir DIR] [resource=value ...] (the `/xce-test-resources [resource=value ...]` form above dispatches to this script.) -- No arguments: defaults to Requirement 1's gap-resource set — the 7 names - `resupdate` is currently missing (`r15m`, `pg`, `ls`, `it`, `swp`, `io`, - `tmp`) — plus a couple of already-supported names for regression coverage - (e.g. `mem` for the numeric path, `type` for the string path), so a default - run also proves the fix didn't disturb existing behavior. +- No arguments: defaults to `DEFAULT_RESOURCE_ARGS` -- Requirement 1's + gap-resource set (`r15m`, `pg`, `ls`, `it`, `swp`, `io`, `tmp`) plus two + already-supported NUMERIC names for regression coverage (`mem`, `cpuf`), + so a default run also proves the `resupdate` fix didn't disturb existing + behavior. Deliberately excludes `type`/`model` (string-typed) from the + default set -- see bug-1574 below. - One or more `resource=value` pairs: checks exactly those, e.g. - `/xce-test-resources r15m=1.5 pg=3`. **Not yet implemented** — task 5.1 - parses and accepts these but does not act on them yet; injection lands in - task 6.1. -- `--dir DIR` — scratch/deploy directory. Default: a fresh directory + `/xce-test-resources r15m=1.5 pg=3`. A name the cluster doesn't recognize + is reported as a rejection and drives the overall verdict to FAIL, + identifying the specific name -- it is never silently dropped or ignored. +- `--dir DIR` -- scratch/deploy directory. Default: a fresh directory generated via `mktemp -d` (xce-deploy-ha's "every invocation is a fresh, self-contained deploy" convention), or `$XCE_RESVERIFY_TOP` when set and `--dir` is not given (same precedence pattern as `xce-test-ha`'s `$XCE_HA_TOP`). -- `--keep-running` — skip PROCESS teardown in cleanup, leaving the +- `--keep-running` -- skip PROCESS teardown in cleanup, leaving the deployment running under `DIR` for further inspection. Log files under - `DIR` are ALWAYS preserved regardless of this flag (see Error handling - below); it only controls whether controller/scheduler/apiserver processes - are killed. + `DIR` are ALWAYS preserved regardless of this flag; it only controls + whether controller/scheduler/apiserver processes are killed. The verdict + is still computed and reported normally either way -- `--keep-running` + does not change PASS/FAIL or the exit code, only what happens to the + processes afterward. Tear the deployment down manually afterward (its PIDs + are visible via `ps`/`pgrep` scoped to `DIR`). - Other overridable env vars (all optional, default to this repo's own meson build dir / `/tmp/apiserver`): `$XCE_RESVERIFY_BUILD_DIR`, `$XCE_RESVERIFY_BIN` (controller), `$XCE_RESVERIFY_SCHED_BIN`, - `$XCE_RESVERIFY_APISERVER_BIN`, `$XCE_RESVERIFY_PEERS` (defaults to a - single-node `localhost:9991` — this skill verifies value propagation, not - raft/HA failover, so a single node is deliberate and avoids - election-timing flakiness; xce-test-ha remains the tool for HA testing). + `$XCE_RESVERIFY_APISERVER_BIN`, `$XCE_RESVERIFY_XT` (the `xce_test` + binary), `$XCE_RESVERIFY_PEERS` (defaults to a single-node + `localhost:9991` -- this skill verifies value propagation, not raft/HA + failover, so a single node is deliberate and avoids election-timing + flakiness; xce-test-ha remains the tool for HA testing). + +Must be invoked under an environment with `nix` on `PATH` -- the script +shells out to `nix develop` itself for the steps that need it, so it does +not need to be pre-wrapped by the caller. ## VERDICT line and PASS/FAIL oracle -Following `xce-test-ha`'s machine-parseable VERDICT convention: +Following `xce-test-ha`'s machine-parseable VERDICT convention, emitted by +`aggregate_verdict()`: ``` -[xce-test-resources] VERDICT resources=7 layers_checked=3 mismatches=0 result=PASS +[xce-test-resources] VERDICT resources=9 layers_checked=3 mismatches=0 unreachable=0 rejected=0 result=PASS ``` -- `resources` — count of resource names checked this run (7 for the - canonical Requirement-1-gap-only run shown above; a default no-args run - that also includes the regression names, or an explicit `resource=value` - list, reports whatever count was actually checked). -- `layers_checked` — always 3 (controller, scheduler, apiserver) for a full - run. -- `mismatches` — count of (resource, layer) pairs where the observed value - did not match the injected value, or the layer was unreachable - (trace-log grep timed out). `result=PASS` iff `mismatches == 0`. -- `result` — `PASS` or `FAIL`. +- `resources` -- total count of resource names checked this run (accepted + and rejected alike). +- `layers_checked` -- always 3 (controller, scheduler, apiserver). +- `mismatches` -- count of (resource, layer) pairs where a trace + line/query DID return a value but it did not match the injected value + (within numeric tolerance; exact string match for non-numeric values). +- `unreachable` -- count of (resource, layer) pairs where the bounded wait + timed out with NO trace line ever observed -- distinct from `mismatches` + per Requirement 4.3/5.4 (the two point at different bugs: a + value-propagation defect vs. a layer that never came up or never logged). +- `rejected` -- count of resource names the injection step itself rejected + (Requirement 1.2) -- never checked at any layer, since nothing was ever + applied anywhere for a rejected name. +- `result` -- `PASS` iff `mismatches == 0 AND unreachable == 0 AND + rejected == 0`; `FAIL` otherwise. -Per-resource, per-layer detail (injected vs. observed value) is reported -above the VERDICT line for anything that mismatched, distinguishing a genuine -**mismatch** (wrong value observed) from **unreachable** (bounded wait timed -out with no matching trace line) — the same distinction Requirement 4.3/5.4 -and design.md's Error Handling require, since the two point at different bugs -(a value-propagation defect vs. a layer that never came up or never logged). - -Alongside the VERDICT line, the skill emits the usual terminal oracle line so -a goal-loop or CI-style caller can branch on it without parsing the VERDICT -line's fields: +Immediately above the VERDICT line, one `FAILURE:` line is printed per +failing (resource, layer) pair or rejected name, naming the specific +resource and layer (or `layer=injection` for a rejection) plus the injected +and observed values -- e.g.: ``` -[xce-test-resources] PASS: 7 resources, 3 layers checked, zero mismatches -[xce-test-resources] FAIL: resource=r15m layer=scheduler mismatch (injected=1.5 observed=timeout) +[xce-test-resources] FAILURE: resource=r15m layer=scheduler status=MISMATCH injected=1.5 observed=1.2 +[xce-test-resources] FAILURE: resource=not-a-real-resource layer=injection status=REJECTED detail="rejected: unknown resource" ``` -PASS ⇒ exit `0`. Any FAIL condition ⇒ non-zero exit. +Then exactly one terminal oracle line: -## Error handling this skill will implement +``` +[xce-test-resources] PASS: 9 resource(s), 3 layers checked, zero mismatches, zero unreachable, zero rejected +[xce-test-resources] FAIL: 2 resource(s), 3 layers checked, 0 mismatch(es), 0 unreachable, 1 rejected -- see FAILURE lines above +``` + +PASS ⇒ exit `0`. Any FAIL condition ⇒ exit `1`. A structural failure before +the aggregation step is reached (deployment failure, worker-join failure, a +malformed `--dir`/argument, etc.) still exits non-zero via this script's +`fail()`/`usage()` (1, or 2 for a usage/argument error) -- unrelated to the +verdict's own `result=` field, since aggregation never ran. + +A full run against the default resource set produces **exactly one** VERDICT +line and a matching process exit code; no controller/scheduler/apiserver +processes remain afterward (unless `--keep-running` was passed); the run's +log files are present on disk regardless. + +### The comparison/aggregation step is self-contained and independently testable + +`aggregate_verdict()` (and its `build_verdict_record()` formatting helper) +takes plain records as function arguments -- no live command, no file I/O, +no global deployment state -- so it can be invoked directly with +hand-written synthetic records, with no live deployment required. This is +what task 8.2's dedicated test harness exercises; source the script +(`source run-resource-verify.sh`) to get `aggregate_verdict()`, +`build_verdict_record()`, and `controller_values_match()` in scope without +triggering `main()` or any live deployment -- the bottom of the file is +guarded (`[[ "${BASH_SOURCE[0]}" == "${0}" ]]`) specifically so sourcing it +has no side effects. + +## Error handling this skill implements Per design.md's Error Handling section: -- **Unrecognized resource name** — `resupdate`'s own follow-up - `xcecli_resval()` verification (task 2.2) reports it as rejected; the skill - treats this as a run-level failure and names the specific rejected - resource(s) in its report. -- **Layer mismatch** — reported with both the injected and observed value, +- **Unrecognized resource name** -- `resupdate`'s own follow-up + `xcecli_resval()` verification (task 2.2) reports it as rejected; the + aggregation step (task 7.1) treats this as a run-level failure and names + the specific rejected resource(s) in its FAILURE lines. +- **Layer mismatch** -- reported with both the injected and observed value, attributed to the specific layer (controller/scheduler/apiserver). - **Layer unreachable** (e.g. apiserver never came up, or the expected - trace line never appears before the bounded wait elapses) — treated the - same as a mismatch for verdict purposes (FAIL), but labeled "timeout" - distinctly from "mismatch" in the report, never a silent skip. -- **Deployment failure** (controller/scheduler/apiserver fails to start) — + trace line never appears before the bounded wait elapses) -- treated the + same as a mismatch for verdict purposes (FAIL), but labeled UNREACHABLE + distinctly from MISMATCH in the report, never a silent skip. +- **Deployment failure** (controller/scheduler/apiserver fails to start) -- the skill aborts before attempting injection, with a clear "deployment - failed at step X" message. Cleanup still runs on this path: a trap (or - equivalent) is installed covering every deployment step from the first - process start onward, not just the post-injection verification path — - deliberately stronger than `xce-deploy-ha`'s own precedent (a separate, - manually-invoked `cleanup.sh` that a mid-deploy failure would otherwise - leave for the caller to run by hand). + failed at step X" message. Cleanup still runs on this path: a trap + registered before the first process starts covers every deployment step + from the first process start onward, not just the post-injection + verification path. -## Not using `context: fork` for the eventual orchestration script +## Known pre-existing bugs relevant to this skill (not fixed here) -Neither this skill's frontmatter nor the invocation instructions below will -set `context: fork`, for the same reason `xce-test-ha` and `xce-deploy-ha` -both call out explicitly in their own `SKILL.md`s: this skill's -`run-resource-verify.sh` will invoke `xce-deploy-ha`'s `start-all.sh` -internally, starting real backgrounded controller/scheduler/apiserver -processes for the duration of the run. `context: fork`'s own teardown sends -SIGHUP through the whole process group on return — the same confirmed-broken -mechanism `xce-test-ha`'s soak driver and `xce-deploy-ha` both hit (bug -confirmed 2026-07-08) — that would silently kill those processes mid-run, -before cleanup or the verdict line ever fires. Dispatch it directly via Bash, -or via the `Agent` tool -(empirically verified safe for this exact pattern — see `xce-test-ha`'s and +- **bug-1572** (open): the live scheduler daemon can double-free/crash if + the SAME resource is re-pushed to it a second time while it's running + (e.g. left alive for a while under `--keep-running`, or a controller + periodic resync). A single run's own injection (one `resupdate` per + resource, one verification pass) does not trigger this, but don't rely on + a `--keep-running` deployment staying up indefinitely once left running. +- **bug-1574** (open): `xce_test` can non-deterministically SEGFAULT on a + string-typed `resupdate` (`-n type`/`-n model`) -- uninitialized-memory + UB, not this feature's own bug. This is why the default resource set is + numeric-only; avoid adding `type`/`model` to a custom `resource=value` + argument until this is fixed. +- **bug-1579** (fixed): host-attrs resources (`cpuf`, `maxcpus`, etc.) used + to false-MISMATCH at the controller layer because the synthetic worker + disconnected before the controller-side check ran. Fixed by nesting the + controller check inside the still-open injecting worker session. + +## Not using `context: fork` for the orchestration script + +Neither this skill's frontmatter nor the invocation instructions above set +`context: fork`, for the same reason `xce-test-ha` and `xce-deploy-ha` both +call out explicitly in their own `SKILL.md`s: `run-resource-verify.sh` +starts real backgrounded controller/scheduler/apiserver processes for the +duration of the run, and `context: fork`'s own teardown sends SIGHUP through +the whole process group on return -- the same confirmed-broken mechanism +`xce-test-ha`'s soak driver and `xce-deploy-ha` both hit -- which would +silently kill those processes mid-run, before cleanup or the verdict line +ever fires. Dispatch it directly via Bash, or via the `Agent` tool +(empirically verified safe for this exact pattern -- see `xce-test-ha`'s and `xce-deploy-ha`'s own `SKILL.md`s), not via a forked Skill-tool context. ## Dependencies - `xce_test`'s worker-context `resupdate`/`join` and client-context `resval` - — resource-name table gap closed (task 2.1/2.2, done). -- `xce-deploy-ha`'s `deploy-ha.sh` / `start-all.sh` — apiserver step and - combined `start-all.sh` done (task 3.1). -- Scheduler and apiserver trace-log lines — done (tasks 4.1 and 4.2). -- This skill's own `run-resource-verify.sh` — deployment step done (task - 5.1); injection + per-layer checks + verdict aggregation not yet built - (tasks 6.1–7.1). + -- resource-name table gap closed (tasks 2.1/2.2). +- `xce-deploy-ha`'s `deploy-ha.sh` / `start-all.sh` -- apiserver step and + combined `start-all.sh` (task 3.1). +- Scheduler and apiserver trace-log lines (tasks 4.1 and 4.2). +- This skill's own `run-resource-verify.sh` -- deployment (task 5.1), + injection + per-layer checks (tasks 6.1-6.4), and verdict aggregation + + cleanup-on-every-path (task 7.1) -- all done. ## Requirements traceability -This stub documents the contract for design.md's "`xce-test-resources` -skill" component (Components and Interfaces → Test Tooling / Bash), covering -Requirements 3.1, 3.2, 4.1, 4.2, 4.3, 5.1, 5.2, 5.4, 6.1, 6.2, 6.3, 6.4, 6.5. -See `.kiro/specs/resource-value-verification/requirements.md` and `design.md` +This document describes design.md's "`xce-test-resources` skill" component +(Components and Interfaces → Test Tooling / Bash), covering Requirements +3.1, 3.2, 4.1, 4.2, 4.3, 5.1, 5.2, 5.4, 6.1, 6.2, 6.3, 6.4, 6.5. See +`.kiro/specs/resource-value-verification/requirements.md` and `design.md` for the full text. diff --git a/.claude/skills/xce-test-resources/run-resource-verify.sh b/.claude/skills/xce-test-resources/run-resource-verify.sh index 74637d62..b46bec9e 100755 --- a/.claude/skills/xce-test-resources/run-resource-verify.sh +++ b/.claude/skills/xce-test-resources/run-resource-verify.sh @@ -5,12 +5,14 @@ # resource value via xce_test's resupdate, confirm it is visible at the # controller, live scheduler, and Go apiserver). # -# resource-value-verification spec, task 5.1+6.1+6.2+6.3+6.4 SCOPE (+ -# bug-1579 follow-up fix): this file currently implements deployment (task -# 5.1), worker join + resource injection (task 6.1), the controller-side -# per-layer verification check (task 6.2), the scheduler-side per-layer -# verification check (task 6.3), AND the apiserver-side per-layer -# verification check (task 6.4) -- stand up a fresh controller + scheduler +# resource-value-verification spec, task 5.1+6.1+6.2+6.3+6.4+7.1 SCOPE (+ +# bug-1579 follow-up fix): this file implements the FULL skill -- deployment +# (task 5.1), worker join + resource injection (task 6.1), the +# controller-side per-layer verification check (task 6.2), the +# scheduler-side per-layer verification check (task 6.3), the apiserver-side +# per-layer verification check (task 6.4), AND the final verdict aggregation +# / VERDICT line / PASS-FAIL oracle / cleanup-on-every-path (task 7.1) -- +# stand up a fresh controller + scheduler # (-vvv) + apiserver (debug logging) deployment via xce-deploy-ha's # deploy-ha.sh + generated start-all.sh, confirm all three components are # actually up, join a SYNTHETIC worker (no real worker daemon process) @@ -34,12 +36,29 @@ # internal_controller_check() function and the top-of-file bug-1579 note # below for why this matters and exactly how it works; DEFAULT_RESOURCE_ARGS # now includes `cpuf` again as proof the fix is real (see that note). -# Verdict aggregation / cleanup-on-every-path (task 7.1) is added by a LATER -# task -- see the "Extension point" marker in main() below for exactly where -# it hooks in. # See .kiro/specs/resource-value-verification/{requirements,design}.md, -# Requirements 2.1, 2.2, 3.1, 3.2, 4.1, 4.2, 4.3, 5.3, 6.1, 6.4, 6.5 for these -# tasks; the full skill covers 3.1-6.5. +# Requirements 2.1, 2.2, 3.1, 3.2, 4.1, 4.2, 4.3, 5.3, 6.1, 6.2, 6.3, 6.4, +# 6.5 for these tasks; the full skill covers 3.1-6.5 and is now complete. +# +# Task 7.1 (this revision): adds the final aggregation step. Combines the +# controller/scheduler/apiserver per-layer results (tasks 6.2-6.4) plus the +# injection accept/reject outcome (tasks 2.1/2.2/6.1) into a single overall +# verdict, via a SELF-CONTAINED comparison/aggregation function +# (aggregate_verdict(), see its own header comment below) that takes plain +# TSV "records" as arguments -- no live command, no file I/O, no global +# deployment state -- so it is directly callable with hand-written synthetic +# records for testing (this task's own sanity-check invocations, and task +# 8.2's dedicated harness, which depends on this exact function/record +# format) as well as from main()'s live-run path. Emits exactly one +# machine-parseable VERDICT line (xce-test-ha's convention) plus the +# standard terminal PASS/FAIL oracle line; exit code is 0 for an overall +# PASS, 1 for an overall FAIL -- this is now the script's REAL exit code, +# not the deployment-only placeholder described in the "Exit 0/Non-zero" +# note below the Usage block (kept for historical/task-5.1-6.4 context, see +# the correction directly beneath it). The bottom of this file is also now +# guarded so it can be `source`d (e.g. by task 8.2's harness) without +# executing main()/internal_controller_check() as a side effect -- see the +# `[[ "${BASH_SOURCE[0]}" == "${0}" ]]` guard at the very end. # # Task 6.1 deliberate deviation from design.md: the default (no-args) # resource set is Requirement 1's gap-resource set (r15m, pg, ls, it, swp, @@ -126,6 +145,8 @@ # coverage (see the top-of-file bug-1574 note for why # the default deliberately excludes type/model). # +# --- SUPERSEDED by task 7.1 (kept for historical context -- this described +# the script's exit code BEFORE the final aggregation step existed) --- # Exit 0 == this task's scope succeeded (deployment stood up, all three # components confirmed up, worker joined, resupdate ran for every requested # resource -- accepted AND rejected outcomes are both a successful run of @@ -148,6 +169,21 @@ # MISMATCH/UNREACHABLE yet (see ANY_CONTROLLER_MISMATCH/ # ANY_SCHEDULER_MISMATCH/ANY_SCHEDULER_UNREACHABLE/ANY_APISERVER_MISMATCH/ # ANY_APISERVER_UNREACHABLE, groundwork for task 7.1's aggregation). +# --- CURRENT (task 7.1, this revision) --- +# Exit 0 == overall verdict PASS: every requested resource name was accepted +# (Requirement 1.2's rejection path never fired) AND every accepted +# resource's controller/scheduler/apiserver observed value matched its +# injected value (see aggregate_verdict() below) -- exactly what the +# script's single VERDICT line's `result=PASS` reports. +# Non-zero (1) == overall verdict FAIL: at least one requested resource name +# was rejected, OR at least one accepted resource's value did not match (or +# was UNREACHABLE/timed out) at some layer -- exactly what `result=FAIL` +# reports, with every specific (resource, layer) failure named in the +# FAILURE lines aggregate_verdict() logs immediately above the VERDICT line. +# A structural failure before aggregation is reached (deployment failure, +# worker-join failure, a resupdate with no observable accept/reject outcome, +# a malformed `--dir`/argument, etc.) still exits non-zero via fail()/usage() +# (1, or 2 for a usage/argument error) -- unchanged from before this task. # set -u @@ -1133,6 +1169,318 @@ query_apiserver_resvals() { done } +# ── Step 8: verdict aggregation, VERDICT line, PASS/FAIL oracle (task 7.1) ── +# Requirement 6.2/6.3/6.4: combine the controller/scheduler/apiserver +# per-layer results (tasks 6.2-6.4) plus the injection accept/reject outcome +# (tasks 2.1/2.2/6.1) into a single overall verdict. Deliberately +# implemented as a SELF-CONTAINED comparison/aggregation function -- +# aggregate_verdict() below takes an array of plain TSV "records" (one per +# checked resource, see the format comment on aggregate_verdict() itself) +# and does its own MATCH/MISMATCH/UNREACHABLE classification via +# controller_values_match() (task 6.2's own pure comparison helper, reused +# here rather than re-parsing/re-trusting each layer's already-rendered +# CHECK_RESULTS string) -- it runs no live command, reads no file, and +# touches no global deployment state, so it is directly callable with +# hand-written SYNTHETIC records for testing (this task's own sanity-check +# invocations during implementation, and task 8.2's dedicated harness, which +# depends on this exact function/record format). main() below builds the +# record array from the ALREADY-POPULATED INJECTED_VALUES/INJECTED_RESULTS +# (task 6.1) and CONTROLLER_OBSERVED/SCHEDULER_OBSERVED/APISERVER_OBSERVED +# (tasks 6.2-6.4) associative arrays for the live-run path; a synthetic +# caller builds the same shape by hand, with no deployment involved at all. +# +# Per design.md's Error Handling, a REJECTED resource name (Requirement 1.2) +# is a run-level failure in its own right -- reported as its own failure +# category ("REJECTED", distinct from a per-layer MISMATCH/UNREACHABLE) and +# always drives the overall verdict to FAIL, independent of the (nonexistent +# -- a rejected name is never actually applied anywhere, see +# query_controller_resvals()/query_scheduler_resvals()/ +# query_apiserver_resvals()'s shared skip rule) per-layer results. + +# Record format (9 fields, separated by VERDICT_FIELD_SEP -- ASCII Unit +# Separator 0x1F, deliberately NOT a tab, see that constant's own comment +# below), one record string per checked resource: +# resourceinjectedinjection_status +# ctrl_reachablectrl_observed +# sched_reachablesched_observed +# api_reachableapi_observed +# +# resource -- resource name. +# injected -- the value that was (or would have been) injected. +# injection_status -- "accepted" or "rejected: " +# (INJECTED_RESULTS' own vocabulary, tasks 2.2/6.1) +# -- anything other than the literal string +# "accepted" is treated as a rejection, independent +# of the reason text. +# *_reachable -- "1" if that layer produced an observed value +# within scope/bounded wait, "0" if it did not +# (timeout, or simply never checked). Ignored when +# injection_status != "accepted" -- nothing was ever +# applied anywhere for a rejected name, so no layer +# is "checked" for it at all (matches tasks +# 6.2-6.4's own skip rule). +# *_observed -- observed value string for that layer, meaningful +# only when the paired *_reachable flag is "1". +# +# A record for a rejected resource still needs all 9 fields present +# (positional read) -- pass "0"/"" for each of the 6 layer fields by +# convention (see build_verdict_record() below); their content is ignored. +# +# VERDICT_FIELD_SEP: ASCII Unit Separator (0x1F), NOT a tab. Found live +# (this task's own synthetic sanity-check run, scenario (c) below) that a +# plain `IFS=$'\t' read` -- the convention every OTHER TSV in this file +# uses -- silently COLLAPSES a genuinely-empty field: bash treats tab (like +# space/newline) as "IFS whitespace" for splitting purposes even when IFS is +# set to ONLY a tab character, so two adjacent delimiters (an empty +# *_observed field, which legitimately happens both for an UNREACHABLE +# layer and for a reachable-but-empty-value MISMATCH, e.g. controller "no +# value" -- see query_controller_resvals()) collapse into one, shifting +# every field after it and silently corrupting the record. None of this +# file's other TSVs (injection-results.tsv, *-check-results.tsv) ever carry +# a genuinely-empty field, which is why this gotcha never surfaced before +# aggregate_verdict()'s record format, the first place in this script where +# an empty field is a legitimate, expected value. 0x1F is not "IFS +# whitespace" and is never expected to appear in a resource name, value, or +# status string, so it splits/preserves empty fields correctly with a plain +# `IFS=$VERDICT_FIELD_SEP read`. +VERDICT_FIELD_SEP=$'\x1f' + +build_verdict_record() { + # build_verdict_record NAME INJECTED INJ_STATUS CTRL_OK CTRL_OBS SCHED_OK SCHED_OBS API_OK API_OBS + # Thin formatting helper so main()'s live-run wiring and any hand-built + # synthetic record share one place that defines field order/count -- not + # itself part of the "self-contained comparison step" (aggregate_verdict() + # is that step); a caller may also just printf/construct the + # VERDICT_FIELD_SEP-joined string directly, this exists purely for + # readability at call sites. + local sep="$VERDICT_FIELD_SEP" + printf '%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s' \ + "$1" "$sep" "$2" "$sep" "$3" "$sep" "$4" "$sep" "$5" "$sep" \ + "$6" "$sep" "$7" "$sep" "$8" "$sep" "$9" +} + +# aggregate_verdict RECORD... +# THE self-contained comparison/aggregation step (Requirement 6.2/6.3/6.4). +# Pure function over its arguments: no network I/O, no file reads, no +# subprocess -- every classification decision is either a direct field +# comparison or a call to controller_values_match() (also pure). Safe and +# deterministic to call with hand-built synthetic records, live-run-derived +# records, or a mix, with no live deployment required either way. +# +# Resets and repopulates these globals on every call (so repeated calls in +# the same shell/test session -- e.g. one per synthetic scenario -- never +# see a stale result from a previous invocation): +# AGG_RESOURCE_STATUS[""] = "PASS" | "FAIL" +# AGG_FAILURE_LINES[] = one human-readable "resource=... layer=... +# status=... ..." string per failing +# (resource, layer) pair or rejected name +# AGG_TOTAL_RESOURCES = count of records passed in +# AGG_MISMATCHES = count of (resource, layer) pairs +# classified MISMATCH +# AGG_UNREACHABLE = count of (resource, layer) pairs +# classified UNREACHABLE +# AGG_REJECTED = count of records whose injection_status +# was not "accepted" +# AGG_RESULT = "PASS" | "FAIL" +# +# Prints one FAILURE line per failing (resource, layer) pair or rejected +# name (naming the specific resource + layer, per Requirement 6.3), then +# exactly one VERDICT line, then exactly one terminal PASS/FAIL oracle line +# -- xce-test-ha's machine-parseable convention, see SKILL.md. +# +# Returns (exit code) 0 when AGG_RESULT=PASS, 1 when AGG_RESULT=FAIL -- so a +# caller (main(), or a synthetic test) can branch directly on the call +# itself: `if aggregate_verdict "${records[@]}"; then ... fi`. +declare -A AGG_RESOURCE_STATUS=() +AGG_FAILURE_LINES=() +AGG_TOTAL_RESOURCES=0 +AGG_MISMATCHES=0 +AGG_UNREACHABLE=0 +AGG_REJECTED=0 +AGG_RESULT="PASS" + +aggregate_verdict() { + AGG_RESOURCE_STATUS=() + AGG_FAILURE_LINES=() + AGG_TOTAL_RESOURCES=0 + AGG_MISMATCHES=0 + AGG_UNREACHABLE=0 + AGG_REJECTED=0 + AGG_RESULT="PASS" + + local rec resource injected inj_status + local ctrl_ok ctrl_obs sched_ok sched_obs api_ok api_obs + local layer ok obs resource_failed + + for rec in "$@"; do + IFS="$VERDICT_FIELD_SEP" read -r resource injected inj_status \ + ctrl_ok ctrl_obs sched_ok sched_obs api_ok api_obs <<< "$rec" + + AGG_TOTAL_RESOURCES=$((AGG_TOTAL_RESOURCES + 1)) + resource_failed=0 + + if [ "$inj_status" != "accepted" ]; then + AGG_REJECTED=$((AGG_REJECTED + 1)) + resource_failed=1 + AGG_FAILURE_LINES+=("resource=$resource layer=injection status=REJECTED detail=\"$inj_status\"") + else + for layer in controller scheduler apiserver; do + case "$layer" in + controller) ok="$ctrl_ok"; obs="$ctrl_obs" ;; + scheduler) ok="$sched_ok"; obs="$sched_obs" ;; + apiserver) ok="$api_ok"; obs="$api_obs" ;; + esac + + if [ "$ok" != "1" ]; then + AGG_UNREACHABLE=$((AGG_UNREACHABLE + 1)) + resource_failed=1 + AGG_FAILURE_LINES+=("resource=$resource layer=$layer status=UNREACHABLE injected=$injected observed=") + elif ! controller_values_match "$injected" "$obs"; then + AGG_MISMATCHES=$((AGG_MISMATCHES + 1)) + resource_failed=1 + AGG_FAILURE_LINES+=("resource=$resource layer=$layer status=MISMATCH injected=$injected observed=$obs") + fi + done + fi + + if [ "$resource_failed" -eq 1 ]; then + AGG_RESOURCE_STATUS["$resource"]="FAIL" + AGG_RESULT="FAIL" + else + AGG_RESOURCE_STATUS["$resource"]="PASS" + fi + done + + if [ "$AGG_TOTAL_RESOURCES" -eq 0 ]; then + # No resources at all is not a legitimate PASS -- nothing was + # actually verified. Structural/degenerate case, not expected from + # main()'s own live-run wiring (inject_resources() already fail()s + # the run before this point if RESOURCE_ARGS somehow resolved empty + # -- DEFAULT_RESOURCE_ARGS is always non-empty), but a synthetic + # caller could legitimately probe this edge, so classify it + # explicitly rather than silently reporting PASS on zero input. + AGG_RESULT="FAIL" + AGG_FAILURE_LINES+=("no resources were checked -- nothing to verify") + fi + + local line + for line in "${AGG_FAILURE_LINES[@]}"; do + log "FAILURE: $line" + done + + log "VERDICT resources=$AGG_TOTAL_RESOURCES layers_checked=3 mismatches=$AGG_MISMATCHES unreachable=$AGG_UNREACHABLE rejected=$AGG_REJECTED result=$AGG_RESULT" + + if [ "$AGG_RESULT" = "PASS" ]; then + log "PASS: $AGG_TOTAL_RESOURCES resource(s), 3 layers checked, zero mismatches, zero unreachable, zero rejected" + return 0 + else + log "FAIL: $AGG_TOTAL_RESOURCES resource(s), 3 layers checked, $AGG_MISMATCHES mismatch(es), $AGG_UNREACHABLE unreachable, $AGG_REJECTED rejected -- see FAILURE lines above" + return 1 + fi +} + +# build_verdict_records_from_tsv TSV_PATH +# Round-2 remediation of the round-1 REJECTED verdict (see the sched_ok/ +# api_ok derivation comment inside for the actual bug/fix). This is main()'s +# own record-building loop (Task 7.1), extracted verbatim from main() into a +# standalone function so it is independently callable -- the round-1 +# reviewer's exact finding was that a hand-built-synthetic-record test of +# aggregate_verdict() alone (task 8.2's harness) could never have caught this +# bug, because the bug lived entirely in main()'s data-marshaling code +# (reading $TOP/logs/injection-results.tsv plus the CONTROLLER_OBSERVED/ +# SCHEDULER_OBSERVED/SCHEDULER_CHECK_RESULTS/APISERVER_OBSERVED/ +# APISERVER_CHECK_RESULTS globals into aggregate_verdict()-shaped records), +# which a bypass-and-call-aggregate_verdict()-directly test never exercises. +# Extracting it here means a synthetic test can source this file, hand- +# populate the same globals + a synthetic TSV, call this function directly, +# and inspect its output -- exercising the EXACT SAME code main() itself +# runs, not a reimplementation of it. No behavior change from the inline +# version: same reads, same globals, same build_verdict_record() calls. +# +# Populates the global VERDICT_RECORDS array (reset on every call, mirroring +# aggregate_verdict()'s own reset-globals-on-every-call convention) with one +# build_verdict_record()-shaped record per line of TSV_PATH. Build one record +# per INJECTED name, in injection order -- read back from +# $TOP/logs/injection-results.tsv, the same durable, order-preserving source +# query_controller_resvals()/query_scheduler_resvals()/ +# query_apiserver_resvals() already read (bash associative arrays have no +# defined iteration order, the TSV does). ANY_CONTROLLER_MISMATCH/ +# ANY_SCHEDULER_MISMATCH/ANY_SCHEDULER_UNREACHABLE/ANY_APISERVER_MISMATCH/ +# ANY_APISERVER_UNREACHABLE (set by the three query_*_resvals() calls) are +# NOT consulted here -- aggregate_verdict() does its own independent +# MATCH/MISMATCH/UNREACHABLE classification straight from the *_OBSERVED +# values via controller_values_match(), the same pure comparison helper each +# query_*_resvals() already uses, so a synthetic caller and this live-run +# path share identical classification logic. +VERDICT_RECORDS=() +build_verdict_records_from_tsv() { + local verdict_tsv="$1" + [ -f "$verdict_tsv" ] || fail "verdict aggregation: $verdict_tsv not found -- inject_resources() must run first" + + VERDICT_RECORDS=() + local vname vvalue vstatus + local ctrl_ok ctrl_obs sched_ok sched_obs api_ok api_obs + while IFS=$'\t' read -r vname vvalue vstatus; do + [ -n "$vname" ] || continue + if [ "$vstatus" = "accepted" ]; then + # ctrl_ok: derived from CONTROLLER_OBSERVED array-key existence. + # This is fine as-is (round-2 review confirmed no change needed) + # -- query_controller_resvals()/internal_controller_check() has + # no separate UNREACHABLE concept at all; a query failure there + # aborts the whole run via fail() rather than leaving a + # placeholder entry, so "no observed value" legitimately means + # MISMATCH for this layer specifically, and array-key existence + # is an accurate proxy for "was queried". + if [ -n "${CONTROLLER_OBSERVED[$vname]+x}" ]; then + ctrl_ok=1; ctrl_obs="${CONTROLLER_OBSERVED[$vname]}" + else + ctrl_ok=0; ctrl_obs="" + fi + # sched_ok/api_ok: round-2 fix (remediation of the round-1 + # REJECTED verdict). query_scheduler_resvals()/ + # query_apiserver_resvals() (tasks 6.3/6.4) unconditionally set + # SCHEDULER_OBSERVED[$vname]/APISERVER_OBSERVED[$vname] for + # EVERY accepted+checked name, including on a genuine + # UNREACHABLE/timeout -- where the value is just set to "" (see + # the `observed=""` branch in both functions). So deriving + # sched_ok/api_ok from *_OBSERVED array-key existence (the old, + # rejected approach) was ALWAYS 1 for a checked name -- it could + # never see the genuine UNREACHABLE status, and aggregate_verdict + # fell through to comparing the injected value against an empty + # string, misreporting UNREACHABLE as MISMATCH. Fix: derive + # sched_ok/api_ok from SCHEDULER_CHECK_RESULTS[$vname]/ + # APISERVER_CHECK_RESULTS[$vname] instead -- both are always + # populated (same unconditional-set guarantee, task 6.3/6.4) with + # one of exactly "MATCH", "MISMATCH: injected=... observed=...", + # or "UNREACHABLE: ..." (see the declare -A comments above each + # array), so a leading "UNREACHABLE:" is the correct, exact + # signal -- not the presence/absence of the OBSERVED entry. + # The `__UNCHECKED__` fallback below is defensive-only (not + # expected to trigger given query_scheduler_resvals()/ + # query_apiserver_resvals() iterate the exact same accepted-name + # list this loop reads from the same injection-results.tsv, so + # CHECK_RESULTS is always populated by the time main() reaches + # this loop) -- but if it ever DID trigger, treating "never + # checked" as ok=1/empty-observed would silently reproduce this + # exact bug class, so it's treated as unreachable instead. + case "${SCHEDULER_CHECK_RESULTS[$vname]-__UNCHECKED__}" in + UNREACHABLE:*|__UNCHECKED__) sched_ok=0; sched_obs="" ;; + *) sched_ok=1; sched_obs="${SCHEDULER_OBSERVED[$vname]-}" ;; + esac + case "${APISERVER_CHECK_RESULTS[$vname]-__UNCHECKED__}" in + UNREACHABLE:*|__UNCHECKED__) api_ok=0; api_obs="" ;; + *) api_ok=1; api_obs="${APISERVER_OBSERVED[$vname]-}" ;; + esac + else + ctrl_ok=0; ctrl_obs="" + sched_ok=0; sched_obs="" + api_ok=0; api_obs="" + fi + VERDICT_RECORDS+=("$(build_verdict_record "$vname" "$vvalue" "$vstatus" \ + "$ctrl_ok" "$ctrl_obs" "$sched_ok" "$sched_obs" "$api_ok" "$api_obs")") + done < "$verdict_tsv" +} + main() { parse_args "$@" resolve_top @@ -1158,18 +1506,21 @@ main() { query_apiserver_resvals - # --- Extension point (task 7.1): aggregate results into a VERDICT line - # plus the terminal PASS/FAIL oracle line, replacing this task's - # placeholder success message and exit code below. - # ANY_CONTROLLER_MISMATCH/ANY_SCHEDULER_MISMATCH/ - # ANY_SCHEDULER_UNREACHABLE/ANY_APISERVER_MISMATCH/ - # ANY_APISERVER_UNREACHABLE (set by query_controller_resvals()/ - # query_scheduler_resvals()/query_apiserver_resvals() above) are - # groundwork for this aggregation -- tasks 6.2/6.3/6.4 themselves - # deliberately do not act on them. --- + # --- Task 7.1: aggregate the controller/scheduler/apiserver per-layer + # results plus the injection accept/reject outcome into one VERDICT + # line + terminal PASS/FAIL oracle line (aggregate_verdict() above). + # build_verdict_records_from_tsv() (round-2 extraction, see its own + # header comment) does the data-marshaling; main() is just a thin + # caller. --- + build_verdict_records_from_tsv "$TOP/logs/injection-results.tsv" - log "PASS (task 5.1+6.1+6.2+6.3+6.4 scope): controller, scheduler, and apiserver confirmed up; worker '$WORKER_HOST' joined and resupdate ran for ${#INJECTED_RESULTS[@]} resource(s) under $TOP (see $TOP/logs/injection-results.tsv for the accept/reject detail per name -- a rejection there is a captured outcome, not a script failure at this stage); controller-side check ran for every accepted resource (see $TOP/logs/controller-check-results.tsv -- a MISMATCH there is also a captured outcome, not a script failure at this stage, see ANY_CONTROLLER_MISMATCH); scheduler-side check ran for every accepted resource (see $TOP/logs/scheduler-check-results.tsv -- a MISMATCH or UNREACHABLE there is also a captured outcome, not a script failure at this stage, see ANY_SCHEDULER_MISMATCH/ANY_SCHEDULER_UNREACHABLE); apiserver-side check ran for every accepted resource (see $TOP/logs/apiserver-check-results.tsv -- a MISMATCH or UNREACHABLE there is also a captured outcome, not a script failure at this stage, see ANY_APISERVER_MISMATCH/ANY_APISERVER_UNREACHABLE); the overall PASS/FAIL verdict lands in task 7.1; cleaning up" - exit 0 + log "aggregating verdict across ${#VERDICT_RECORDS[@]} resource(s) under $TOP; cleanup (trap-based, same mechanism as a mid-deployment abort) fires on exit below regardless of PASS/FAIL, unless --keep-running was passed -- logs under $TOP/logs are preserved either way" + + if aggregate_verdict "${VERDICT_RECORDS[@]}"; then + exit 0 + else + exit 1 + fi } # Hidden internal entry point (bug-1579 fix): inject_resources()'s `!` @@ -1179,10 +1530,22 @@ main() { # (which has no knowledge of this flag and would otherwise reject it as an # "Unknown argument") -- runs only internal_controller_check(), none of the # normal deploy/inject/trap flow. -if [ "${1:-}" = "--internal-controller-check" ]; then - shift - internal_controller_check "$@" - exit $? -fi +# +# Guarded so this file can be `source`d (e.g. a synthetic test harness that +# wants aggregate_verdict()/controller_values_match()/build_verdict_record() +# in scope, task 8.2's use case) without executing main() or +# internal_controller_check() as a side effect -- ${BASH_SOURCE[0]} (this +# file's own path) only equals ${0} when the script is the thing actually +# being run (directly executed, or `bash run-resource-verify.sh ...`, +# including the `--internal-controller-check` recursive invocation above, +# which is a fresh `bash "$SELF_SCRIPT" ...` process, not a source), not +# when it's sourced into another shell/script. +if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then + if [ "${1:-}" = "--internal-controller-check" ]; then + shift + internal_controller_check "$@" + exit $? + fi -main "$@" + main "$@" +fi diff --git a/.kiro/specs/resource-value-verification/tasks.md b/.kiro/specs/resource-value-verification/tasks.md index 50a770d9..14b0b46e 100644 --- a/.kiro/specs/resource-value-verification/tasks.md +++ b/.kiro/specs/resource-value-verification/tasks.md @@ -93,7 +93,7 @@ - _Boundary: xce-test-resources skill (apiserver check) — same file as 6.2/6.3, sequential_ - [ ] 7. Integration: verdict aggregation, reporting, and cleanup -- [ ] 7.1 Aggregate layer results, emit the verdict, and clean up +- [x] 7.1 Aggregate layer results, emit the verdict, and clean up - Combine the controller/scheduler/apiserver check results into a single per-resource, per-layer comparison, implemented as a self-contained comparison step that accepts a set of (resource, injected value, per-layer observed value) records — so it can be invoked directly with synthetic records for testing (task 8.2), not only reachable from a live run - Emit a machine-parseable VERDICT line (following `xce-test-ha`'s convention) plus the standard terminal PASS/FAIL oracle line, naming every layer that mismatched or was unreachable (timed out) - Support checking multiple resource names in one run, defaulting to the resource names added in task 2.1 when none are supplied @@ -116,6 +116,7 @@ - _Depends: 7.1_ ## Implementation Notes +- Task 7.1 (round 1 REJECTED, round 2 fixed): `aggregate_verdict()`/`build_verdict_record()` (VERDICT_FIELD_SEP=`$'\x1f'`, ASCII Unit Separator — bash's IFS tab-splitting silently collapses adjacent empty fields even with IFS set to only a tab, bug-1581) are the self-contained, synthetic-testable comparison step task 8.2 needs. `main()`'s live-data wiring is `build_verdict_records_from_tsv()` — derive a layer's "ok" status from `SCHEDULER_CHECK_RESULTS[$name]`/`APISERVER_CHECK_RESULTS[$name]`'s STRING CONTENT (`UNREACHABLE:*` prefix check), never from array-key existence — tasks 6.3/6.4 always set the key, even on a genuine timeout (just with an empty value), so key-existence can never distinguish UNREACHABLE from MISMATCH (bug-1582). `CONTROLLER_OBSERVED`'s key-existence check IS correct for the controller layer specifically (task 6.2 has no separate UNREACHABLE concept — a query failure there aborts the whole run). VERDICT line format: `[xce-test-resources] VERDICT resources= layers_checked=3 mismatches= unreachable= rejected= result=PASS|FAIL` — `rejected=` is an intentional addition beyond design.md's illustrative template (Requirement 6.3 needs rejected names attributable in the machine-parseable line itself); `layers_checked=3` is a fixed constant, not a per-run computed count. SKILL.md fully rewritten to describe the now-complete, functional tool. - Task 6.4: `poll_apiserver_log()`/`query_apiserver_resvals()` mirror task 6.3's scheduler-check pattern exactly (same bounded-wait/batched-polling shape). The apiserver's zap `console` encoder renders structured fields as a trailing JSON object (`resource-location stored {"resource": "r15m", "host": "...", "value": 1.5}`), not classic `key=value` text — grep for the message string, then parse the trailing JSON block. All three layers (controller/scheduler/apiserver) confirmed simultaneously MATCH on the full default set including `cpuf`, proving the bug-1579 fix holds end-to-end. - **bug-1579 is now FULLY fixed** (retrofit on top of tasks 6.1/6.2, not a numbered task — user-requested follow-up). `inject_resources()`'s worker-context `xce_test` script now inserts a `!` line between `resupdate` and `quit`: `! bash