test(e2e): cover install key enrollment and access policy decisions - #7079
Conversation
|
Claude finished @gustavosbarreto's task in 28m 56s —— View job Code Review CompleteReviewed 4 files. 10 inline issues posted — Standards: 1 violation, 5 judgement calls. Spec: axis did not run. Correctness: 4. Standards
Correctness
The other three: the approval wait loop never reads SpecThe Spec axis did not run: the PR body carries no issue reference and neither commit has a Additional notes
Verified as correct, for the record: every response shape (all four endpoints return bare arrays, not paginated envelopes), every asserted 200, the auth on To request another review round, comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
All 10 inline items are addressed in 85ab415..c4426d4, one commit each. Won't address from the summary: the non-blocking banner send with its buffer of 8 and the single-caller |
c4426d4 to
582f594
Compare
|
/review |
|
Claude finished @otavio's task in 17m 21s —— View job Code Review CompleteReviewed 5 files. 5 inline issues posted — Standards: 0 violations, 4 judgement calls. Spec: axis did not run. Correctness: 1. All ten findings from the previous round hold. I re-derived each fix against the server source rather than taking it on trust, and the two refactors they produced — the StandardsNo hard violations. Every surviving comment earns its Correctness
SpecThe Spec axis did not run: the PR body references no issue and no commit carries a Additional notesFour agent findings were dropped in validation, and the reasons are worth stating since three of them re-open settled threads:
Verified as correct, for the record, against the source rather than assumed: the log substrings really do match the emitted format ( One thing that is not a defect but survives from round 1 unchanged: this takes the No cross-repo impact — the diff touches only To request another review round, comment |
582f594 to
93e4305
Compare
The suite registered every agent with a tenant id, so install keys had no coverage on the wire at all, and the identity model was exercised only for "a password is not offered" and "an enrolled identity connects". Both features were validated by hand against a restored production dump; these cases are that validation, automated. Each case stands up its own stack. Sharing one would be cheaper, but a namespace's access mode, its policies and its enrolled identities are all namespace-wide state, and a case that deletes every policy would decide the outcome of its neighbours. Three things the code cannot say: The subtests take context.Background() rather than t.Context() because Up captures the context inside the closure it stores as Down, and Down runs from t.Cleanup, after t.Context() is already cancelled. Every teardown would fail its require.NoError. An authorization refusal reaches the SSH client as a bare auth failure: the reason and the policy name go to the server's log, and the banner carrying them is only sent to web sessions. Asserting on the log is the only way to tell "refused for the right reason" from "refused because the tunnel was not up yet". The approval code is read from the link in the banner rather than from the "Security code" line, which groupCode splits with a space, and rather than from an API, because approvals are only addressable by a code the caller already holds. The unknown-key case dials from a goroutine and puts a deadline on both the banner and the result: ssh.Dial applies its timeout to the TCP connect only, so a handshake left waiting on an approval would otherwise hang until the whole test binary hits its timeout.
The install-key and access-policy cases add eight stacks to the suite. They cost 232s measured locally — around 30s each, because RemoveImagesAll drops the tags while the BuildKit cache keeps the layers, so the rebuild that follows is nearly free. That is a small addition, but test_timeout was exactly 45m on a job already taking about that long, so it had no room to absorb it.
93e4305 to
f08b990
Compare
A stacked PR never got an automatic review. #7084 was opened against the branch of #7079 at 22:01:33, #7079 merged two seconds later, and GitHub retargeted #7084 onto master at 22:01:37. The opened event carried the old base, so branches: [master] filtered it out and no run was ever queued; the retarget arrives as edited, which nothing listened for; and the force-push that followed is synchronize, which stays excluded so the author drives re-review. The PR fell through every trigger and only got a review when its author asked for one by hand. edited covers the retarget, gated on changes.base so editing a title or a description does not spend a review. ready_for_review closes the sibling hole, where a PR opened as a draft is filtered by the draft check at open and has nothing to re-trigger it once it is ready. The same guard has to sit on cancel-in-progress, not only on the job. A run joins the concurrency group and cancels the one in flight before any job-level if: is evaluated, so gating the job alone would let a title edit cancel a running review and then skip its own, leaving no review at all — and it would defeat this change's own case, where a retarget starts a review that the author's next title edit kills. The other triggers cannot produce an edited action, so their runs keep cancelling as they do today. The comment saying the review runs only on open went with the change that made it false.
A stacked PR never got an automatic review. #7084 was opened against the branch of #7079 at 22:01:33, #7079 merged two seconds later, and GitHub retargeted #7084 onto master at 22:01:37. The opened event carried the old base, so branches: [master] filtered it out and no run was ever queued; the retarget arrives as edited, which nothing listened for; and the force-push that followed is synchronize, which stays excluded so the author drives re-review. The PR fell through every trigger and only got a review when its author asked for one by hand. edited covers the retarget, gated on changes.base so editing a title or a description does not spend a review. ready_for_review closes the sibling hole, where a PR opened as a draft is filtered by the draft check at open and has nothing to re-trigger it once it is ready. The same guard has to sit on cancel-in-progress, not only on the job. A run joins the concurrency group and cancels the one in flight before any job-level if: is evaluated, so gating the job alone would let a title edit cancel a running review and then skip its own, leaving no review at all — and it would defeat this change's own case, where a retarget starts a review that the author's next title edit kills. The other triggers cannot produce an edited action, so their runs keep cancelling as they do today. The comment saying the review runs only on open went with the change that made it false.
2 participants
Install keys and the identity access mode had no end-to-end coverage. Every agent in
the suite registered with a tenant id, so the install key path was never exercised on
the wire, and the identity model was covered only by "a password is not offered" and
"an enrolled identity connects" — nothing about what the Access Policies decide.
Install key enrollment (
tests/install_key_test.go). The agent enrols with a keyand no tenant id, which is what proves the key resolves the namespace on its own.
automatickey lands the device acceptedmanualkey leaves it pendingAccess policy decisions (
tests/identity_access_test.go).with
source: approvaland resumes the loginno_grantEach case stands up its own stack: access mode, policies and identities are all
namespace-wide state, and the case that deletes every policy would otherwise decide
the outcome of its neighbours. The eight cases cost 232s locally, ~30s each —
RemoveImagesAlldrops the image tags but the BuildKit cache keeps the layers.test_timeoutgoes 45m → 60m, since it sat exactly at the length of the job.NewMemberis new in theenvironmentpackage: the API route raises an invitationinstead of a membership on editions without direct membership, so it goes through
admin namespace member add.