test(employment): cover ReassignOrgUnit fail-closed refusals - #802
Conversation
jason931225
left a comment
jason931225
left a comment
There was a problem hiding this comment.
FAIL size bar at 9b9f08c.
backend/crates/ontology/canonical-adapter-postgres/tests/employment_port_as_runtime_role.rs is 2356 lines after +237. The 100–300 bar applies to the touched test file. Four fail-closed ReassignOrgUnit cases belong on this owner, but they have to land in siblings under 300 (or a split of this file), not another 200+ on a 2k-line test.
Other bars: existing owner file (pass), one capability (pass), executed-tests-baseline 31→35 recorded same-change (pass), no new required check (pass).
Not APPROVE. Re-review after the test file is under 300.
jason931225
left a comment
jason931225
left a comment
There was a problem hiding this comment.
FAIL DRY at f8f7556. Size passes (sibling 276; employment_port_as_runtime_role.rs not in the diff; not a rename of the god file). Baseline +4 recorded same-change.
Second copies in tests/employment_reassign_as_runtime_role.rs:
runtime_role_pool— existing owner isconsole_platform_test_support::runtime_role_poolseed_employee,fixture,appointed,execute,command,at,count_rows,legacy_head— existing owner istests/employment_port_as_runtime_role.rs
Use the existing owners. Do not add a third path. The four ReassignOrgUnit cases can stay in this sibling.
Not APPROVE. Re-review after the second copies are gone.
jason931225
left a comment
jason931225
left a comment
There was a problem hiding this comment.
FAIL DRY at f8f7556 (blast-radius bar). Size still passes: sibling 276; employment_port_as_runtime_role.rs not in the diff; not a rename.
Prior comment over-cited console_platform_test_support and other-port runtime_role_pool copies. Those are look-alikes across ports. Not a miss. Do not add a shared surface.
Still fail: second copies in tests/employment_reassign_as_runtime_role.rs of employment-owned fixtures. Existing owner is tests/employment_port_as_runtime_role.rs. A change to one must land with the other.
Second copies: runtime_role_pool, seed_employee, fixture, appointed, execute, command, attributes, at, count_rows, legacy_head.
Not copies: stamp_legacy_org_unit, begin_org_armed_tx, drive_reassign. The four ReassignOrgUnit cases stay in this sibling.
Use the existing owner. Do not invent a third path.
Not APPROVE. Re-review after the second copies are gone.
jason931225
left a comment
jason931225
left a comment
There was a problem hiding this comment.
PASS at f8f7556. Size: sibling employment_reassign_as_runtime_role.rs is 276; employment_port_as_runtime_role.rs not in the diff; not a rename.
DRY: employment fixture copies in this sibling are accepted. Integration tests cannot import from the 2k file; folding them there grows it; a tests/common or third path is a new shared surface. Not a fail.
Bars checked: size, existing-file / not a rename, one capability, recorded baseline +4, no new required check. Not APPROVE.
27869d0 to
f8f7556
Compare
jason931225
left a comment
jason931225
left a comment
There was a problem hiding this comment.
Follow-up at f8f7556. Size pass (sibling 276).
DRY copies accepted, including runtime_role_pool. That crate's Cargo.toml already records why: console-platform-test-support is not a dep because a new name rewrites backend/Cargo.lock. The helper is inlined on purpose. Employment fixtures cannot be imported across integration tests without a new share path or growing the 2k file — both worse blast radius. Not a fail.
Not APPROVE.
f8f7556 to
a904304
Compare
jason931225
left a comment
jason931225
left a comment
There was a problem hiding this comment.
PASS at a904304 (rebase onto current main only). Size: sibling employment_reassign_as_runtime_role.rs is 276; employment_port_as_runtime_role.rs not in the diff; not a rename. Baseline +4 recorded same-change.
DRY copies accepted (same as f8f7556): runtime_role_pool is inlined on purpose; employment fixtures cannot be imported across integration tests without a new share path or growing the 2k file.
Bars checked: size, existing-file / not a rename, recorded baseline growth, no new required check. Not APPROVE.
a904304 to
9428832
Compare
756c226 to
d0980a6
Compare
…re (#819) Stacked on #818. **Review the top commit only.** ## The problem `check:ci-preflight` was an **11-link `&&` chain** — the shell stops at the first failing gate and the remaining ten never run. Three broken gates meant **three full CI cycles**, and at this repo's ~20 minute wall clock that's an hour to learn what one sweep already knew. #813 and #802 each burned a cycle this way today. ## What was already right The workflow **layer** was already fail-slow and stays untouched: 80 steps in `ci.yml` carry `!cancelled()`, and `scripts/ci-collect-failures.mjs` re-asserts the job red naming every failing step. That sweep stopped at the *step boundary* — inside one step, `&&` still short-circuited. This closes the gap one level down, in the same shape, with the precedent `tools/ci/cargo-test-runner.sh` already states: *"one red binary never hides the rest"*. ## Red is preserved The sweep's exit is `every(ok)`, so one red gate keeps the whole run red. Fail-slow becoming fail-**open** would be worse than the problem it solves, so that's mutation-proven rather than asserted. ## A companion fix that was not optional `check-mjs-dark-suites.mjs` discovers reachability by scanning **package.json script text**. Moving the test commands into a manifest made **20 live suites report as dark** — and the natural response to that is to baseline them, retiring real coverage on the strength of a scanner blind spot. The scanner now reads the sweep manifest as the execution path it is. ## Mutation proof Two synthetic red gates injected at positions 2 and 6: ``` FAIL synthetic-red-A <- the old chain ended the run here ...ten more gates still ran... FAIL synthetic-red-B gate-sweep: 12 passed, 2 failed gate-sweep: FAILED synthetic-red-A, synthetic-red-B exit=1 ``` Removed → exit 0. ## Verification `npm run check:ci-preflight` → **12 passed, 0 failed**, exit 0 · `gate-sweep.test.mjs` → **8 pass, 0 fail** · `check-mjs-dark-suites --strict` → dark_count 0, exit 0 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
d0980a6 to
6d8e716
Compare
jason931225
left a comment
jason931225
left a comment
There was a problem hiding this comment.
FAIL size: backend/crates/ontology/canonical-adapter-postgres/tests/employment_reassign_as_runtime_role.rs is 335 lines (bar 100–300). Owner employment_port_as_runtime_role.rs is not in the diff (did not grow). Sibling is present. CoS-accepted DRY copies are not a fail. Hold APPROVE.
jason931225
left a comment
jason931225
left a comment
There was a problem hiding this comment.
PASS at 2ac222f. Size: employment_reassign_as_runtime_role.rs 299 (unbound/mixed); employment_reassign_identity_as_runtime_role.rs 142 (identity/unknown dest). Owner employment_port_as_runtime_role.rs is not in the diff. Baseline +2/+2 recorded same-change. CoS-accepted DRY copies are not a fail. No new required check. Hold APPROVE.
…w edge `otp_redeem_rate_limit_wires_up_on_real_clock_path` drives ten requests expecting 401 and asserts the eleventh is 429. The limiter buckets into a FIXED one-minute TUMBLING window -- `floor_to_window` in crates/platform/auth-rest/src/lib.rs floors to `unix - unix.rem_euclid(60)` -- not a sliding one, so all eleven must land inside the SAME window. When the loop straddles a minute boundary the counter resets mid-loop and the eleventh request is the new window's first, returning 401: assertion `left == right` failed: the trusted ingress identity must select the first per-IP bucket Each of the eleven round-trips to PostgreSQL, so on a loaded runner the test occupies a real fraction of the window and fails whenever it starts late in one. It did exactly that on run 32225725163, failing #802 -- a pull request that changes nothing near auth -- and blocking it behind an unrelated red. The sibling named in this test's own doc comment, `rate_limit_trips_at_cap_and_resets_after_window`, drives `now` directly and has no such exposure. This one exists to prove the REAL clock path is wired, so it cannot inject a clock -- but it can decline to start near a boundary. It now waits for a fresh window when fewer than 30 seconds remain in the current one. The assertion itself is unchanged: this removes a timing dependency the test never meant to have, not a behaviour it meant to check. Verified: cargo check -p console-app --tests, cargo fmt --check and cargo clippy --tests all clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unbound legacy heads, mixed units, identity reassigns, and unknown destinations must refuse without writing, not audit as a successful move.
The four unbound/identity/unknown-destination cases move to employment_reassign_as_runtime_role.rs so the 2k employment file does not grow past the size bar.
…enerate
The new `tests/employment_reassign_as_runtime_role.rs` was added without a
matching entry in TEST_RESOURCE_REQUIREMENTS, so the generated-face writer
refused:
ValueError: test resource metadata must match generated targets:
missing [('console-ontology-canonical-adapter-postgres', 'test.integration',
'tests/employment_reassign_as_runtime_role.rs')]
That check is one-to-one in BOTH directions -- a new test with no declaration
fails, and a declaration whose test disappeared fails too -- so the Buck face
cannot drift from the test tree in either direction.
The test exercises ReassignOrgUnit fail-closed refusals through the runtime role,
so it needs `postgres`, matching its sibling `employment_port_as_runtime_role.rs`.
Regenerated the first-party face (173 BUCK files); the only change is the new
target in the ontology canonical adapter.
Verified: tools/buck/generated-face-first-party.sh -> generated 173 files, no
validation error; npm run check:ci-preflight -> exit 0;
check-postgres-cargo-map -> OK.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`check:executed-tests` refused the branch:
1 test binary(ies) execute nowhere and are not named in
docs/program/executed-tests-baseline.json:
backend/crates/ontology/canonical-adapter-postgres/tests/employment_reassign_as_runtime_role.rs
Wire each into a workflow step, or the repository has 1 more test(s) that cannot fail.
The generated Buck face was regenerated earlier in this branch, so the TARGET
existed -- but no shard selected it, because tools/ci/postgres-cargo-map.json is
what decides which targets a PostgreSQL shard runs, and the new binary was absent
from it. A test that builds and never executes is the false green this ratchet
exists to catch.
Wired rather than baselined. Baselining would have satisfied the gate by
declaring the test permanently dark, which is the opposite of what a new
fail-closed refusal test is for.
The entry mirrors its sibling employment_port_as_runtime_role in the same crate.
`measured_seconds` is deliberately ABSENT: the test has never run in CI, so there
is no measurement, and inventing one would have the duration partitioner
(#815/#810) balance shards on a number nobody took.
Workflow targets 209 -> 210; facets app=56 platform=39 ontology=27 domain-a=44
domain-b=44.
Verified: node tools/ci/check-postgres-cargo-map.mjs -> OK (210 entries);
npm run check:executed-tests -> exit 0, and employment_reassign no longer listed
(the residual "executed nowhere: 1" is seaweedfs_worm.rs, already baselined on
main); npm run check:ci-preflight -> exit 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`#![cfg_attr(rustfmt, rustfmt::skip)]` was the only occurrence of `rustfmt::skip` anywhere under `backend/`. The repository's formatting invariant is `cargo fmt --all -- --check`, and no gate forbids the skip attribute, so this file permanently opted itself out of a rule every other backend source obeys -- including its own sibling, `employment_port_as_runtime_role.rs`. The gate was green only because the file told rustfmt not to look: removing the attribute and re-running `cargo fmt --all -- --check` failed with a 201-line diff. Formatted here, so the file is covered by the same gate as everything around it. No test logic changed; `cargo check -p console-ontology-canonical-adapter-postgres --tests` is clean and `cargo fmt --all -- --check` now exits 0 with no exemption. Found by adversarial review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reassign sibling is 335 lines after rustfmt; the size bar is 100–300. Keep unbound + mixed-peer in employment_reassign_as_runtime_role.rs (299). Move identity (source==target) and unknown destination to employment_reassign_identity_as_runtime_role.rs (142). test_attribute_baseline: 4 → 2 on the existing sibling, +2 on the new one. Declared postgres resource, regenerated the first-party face, and wired the new binary into postgres-cargo-map (workflow 210 → 211) so it executes. Verified: both siblings 100–300; employment_port_as_runtime_role.rs unchanged; four fail-closed cases still present; no tests/common, no new crate, no Cargo.lock; check-postgres-cargo-map OK; check:executed-tests OK.
2ac222f to
4aed5fd
Compare
1 participant
Summary
ReassignOrgUnitalready refuses an ACTIVE legacy head with noemployment_source_bindingsrow (UnboundEmployeeForTransfer). That path was untested.source == target) isBlocked, not a successfulmoved=Nno-op.UnknownOrgUnit, not silentOk(0).#[sqlx::test]attribute baseline for this file only: 31 → 35. No new required check, no skipped/weakened gate.Test plan
cargo test -p console-ontology-canonical-adapter-postgres --test employment_port_as_runtime_role-- --listUnboundEmployeeForTransfermatch arm turns the first two redOk(0)for unknown destination turns the fourth red