Follow-up from "test(managementapi): cover the container upstream reads and associate part 12/15 (S17 Phase 6 plan: 42/42)"

The following discussions from !2857 (merged) should be addressed:

  • @vespian_gl started a discussion:

    non-blocking: This comment says "The update writes before its one read", but the handler reads first.

    handleUpdateUpstream calls repositionedUpstreamReadable before arm.store.Reposition, and that helper calls arm.store.Upstreams first. Then settledUpstream reads the list again. So the update reads, writes, and reads again. S17 requires this order: "The POST and PATCH evaluate the check before they write".

    As a result, a gate that the update skipped shows in rig.store.Lists(), which the case asserts on the line above. The Repositions() assertion does no harm, but the reason the comment gives for it is false. A reader who trusts the comment can accept a later change that moves the check after Reposition. That change breaks the S17 rule that a 503 applies nothing. Guardrail 17 in AGENTS.md requires a comment to describe the code as the change leaves it.

    The comment should go, or it should match the code. For example: "The update reads the association before it moves it, so Lists already catches a skipped gate; Repositions pins that no move reached the store either."

    This is a bot message 🤖 — /mergeable

  • @vespian_gl started a discussion:

    non-blocking: Two assertions in this file check less than their messages say.

    1. This require.Equal on 201 says "the mix must be admitted". But fakeVirtualUpstreams.Associate accepts any upstream, and handleAssociateUpstream has no format or family gate before arm.store.Associate. So this status cannot fail on admission, and a store change that refuses the docker/oci mix does not fail here. TestContainerVirtualUpstreamStore_Associate_AdmitsTheDockerOciMixBothWays in the datastore suite tests the admission. This test pins that the associate renders the upstream's stored format.
    2. In TestContainerUpstreams_List_VerdictFailure_FailsClosed, the unavailable row (line 284) sets no wantLog. So the assert.Contains at line 315 becomes Contains(logs, "") on that row, which is always true. Its message says that the 500 log line names the lengths, but that row is a 503.

    The first message should say what the test checks: the associate renders the upstream's stored format. The log assertion should run only when tc.wantLog != "", or the 503 row can get its own expected fragment if that path writes one.

    This is a bot message 🤖 — /mergeable

  • @vespian_gl started a discussion:

    non-blocking: This test sends the same requests over the same state as the the sibling container format row at line 399.

    Under the oci arm, that row stores a docker virtual repository and sends all five routes under /oci. Under the docker arm, this test also stores a docker virtual repository and sends all five routes under /oci. The same pair holds for the other arm. The doc comment says this test reads "the same rule ... from the route rather than the row", but no handler change fails one test and passes the other.

    This test adds a seeded association row, a Dissociates() assertion, and assertNoAuditEventsOnSink. It does not assert Lists() or the CodeNotFound envelope, which the table does. So a reader sees two directions of coverage where there is one, and the copy with fewer checks is the one that claims to add a direction.

    One of the two should go. You can remove the sibling row from the table and add Lists() and the envelope check here. Or you can remove this test and move its Dissociates() and audit assertions into the table. If you keep both, the doc comment should not claim a separate direction.

    This is a bot message 🤖 — /mergeable

  • @vespian_gl started a discussion:

    non-blocking: Part 11 already defines containerArm{segment, format}, containerArms(), and containerArmRow(t, suffix, position, kind, format) in container_upstream_arms_test.go.

    This file adds containerUpstreamArm, containerUpstreamArms(), and containerUpstreamRow with the same signature. It also adds containerUpstreamTestID, a copy of npmUpstreamTestID with a different prefix, and containerUpstreamRoute, which has the same fields as npmUpstreamRoute. The Maven suite reuses the npm route type: mavenUpstreamRoutes returns []npmUpstreamRoute. Also, the rig's arm field (line 82) is set at line 126 and nothing reads it.

    Nothing fails today. But with two arm tables for the same two segments in one package, a change to one table does not reach the other suite, and that suite stops testing the new arm without failing.

    The container suites should share one arm table. You can add the sibling fields to part 11's containerArm and build the rows with containerArmRow. containerUpstreamRoutes can return []npmUpstreamRoute, as the Maven suite does. The unread arm field should go, or a test should use it.

    This is a bot message 🤖 — /mergeable