Skip to content

test(sandbox): name verify_record(expected_nonce=...) in the docstring - #329

Open
imran-siddique wants to merge 1 commit into
mainfrom
fix/sandbox-test-docstring-nonce
Open

test(sandbox): name verify_record(expected_nonce=...) in the docstring#329
imran-siddique wants to merge 1 commit into
mainfrom
fix/sandbox-test-docstring-nonce

Conversation

@imran-siddique

Copy link
Copy Markdown
Member

Follow-up to #315, closing the one clause @lywinged flagged and explicitly declined to hold that merge for.

After #315 the nonce field docstring names verify_record(expected_nonce=...), while the test docstring beside it still said nonce is never checked "against anything else". A verifier can compare it against a value it chose; what that establishes is freshness rather than key binding, which is the distinction the surrounding paragraph is drawing. The docstring now says that.

Also drops a doubled unrelated, in the same sentence.

Docstring only. No test behaviour changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01X27MBo6tGVw1UmmchuGFnW

Follow-up to #315, closing the one clause @lywinged flagged and declined to
hold the merge for.

The field docstring now names `verify_record(expected_nonce=...)`, while this
test docstring beside it still said `nonce` is never checked "against anything
else". A verifier can compare it against a value it chose; what that
establishes is freshness rather than key binding, which is the distinction the
rest of the paragraph is making. Says that instead.

Also drops a doubled "unrelated,".

Signed-off-by: Imran Siddique <45405841+imran-siddique@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X27MBo6tGVw1UmmchuGFnW
@imran-siddique
imran-siddique requested review from a team and lywinged as code owners September 11, 2026 15:25

@lywinged lywinged left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved on 73a472b.

Stripped of every docstring, tests/test_sandbox_adapter.py parses to the same tree on this head and on df0120b. The four changed lines, 247 to 250, sit inside the docstring that runs from 240 to 256, so "Docstring only" is measured. "unrelated" occurs twice in that docstring on df0120b and once here, which is the doubled word the body names. No CHANGELOG entry, as with #270, #278 and #327.

The check the new sentence names is there: verify_record takes expected_nonce as a keyword-only argument and compares it with runtime.nonce through hmac.compare_digest, sign.py lines 706 to 709. The test's own two records, built from the identical attestation and signed with two fresh keys, both verify, each against its own key, once expected_nonce="claimed-binding-to-key-A" is added to the call. The check is satisfied under either key and says nothing about which one. A different expected value refuses both with record runtime.nonce does not match expected_nonce. That runs the key-binding half; the freshness half is the spec's classification rather than a run: spec/trace-v0.2.md puts challenge-nonce binding under the Freshness paragraph of 3.2.2, and sign.py labels the check the same way at line 705.

The four CI steps with the hash-pinned install, on 3.11 and on 3.12: green, 1404 passed, 1 skipped, the sandbox file 45 passed. Merged onto main at 10fcba4: 1410 passed, 1 skipped on both interpreters, and no file here is one #322 or #327 touched. GitHub's merge ref for this PR, e05a3a9, is 73a472b onto df0120b, so the run onto 10fcba4 is the one that includes those two.

One correction to my round two on #315, which called this test docstring "the one line left saying the other". It was not the only one. The SandboxAttestation class docstring carries the same closing clause at sandbox.py lines 125 to 126, "against any key, or against anything else", in the same shape as the sentence this PR corrects. It arrived in the same diff, and both my reviews there passed over it. So the class denies in that sentence the check its own field docstring names at line 146. The module docstring's own denials stay true: "nothing here checks a quote, a signature, or a nonce" at line 35 and "nothing here ties" at line 52 are scoped to the adapter, and "nothing in this codebase does" at lines 64 to 65 to the key binding. A case-insensitive sweep of this head for "anything else" gives five lines; the other four, in spec/registry-anchor-v1.md, content_marking.py, sign.py and tests/test_public_functions_raise_what_they_document.py, are not about the nonce. For lines 124 to 126, something like, in the field docstring's own words:

    that binding on its own -- ``nonce`` is carried through to the record verbatim, on
    trust, and is not checked against ``measurement`` or against any key; downstream
    ``verify_record(expected_nonce=...)`` compares it only with a value the verifier
    already knows. If you bind your quote's challenge to your signing key yourself,

Not this PR's line and not a reason to hold it. Yours to take here or in a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants