Repository navigation
Conversation
|
This pull request's description says it closes #51, which is on the project roadmap. When it merges, GitHub closes that issue and the roadmap counts it as delivered. Please confirm the change delivers the whole issue before merging. |
00d35c4 to
3df0745
Compare
rocklambros
left a comment
There was a problem hiding this comment.
Raghuvar, your §9 text answers the second gap in issue #51. The issue says the spec never tells us how a human approver meets the rule, and your text does: out of band, through the deployment's approval interface. Your schema edit changes only a description, so all 12 payloads I tested get the same result as before. The merged tree on integration 19b1197 passes pytest (924 passed, 1 skipped), mkdocs build --strict, and both Bun versions with the same counts as before.
This PR doesn't close issue #51, though. The first gap is "auth is optional even for the approvers it is scoped to". The issue's example is { "type": "service", "id": "...", "endpoint": "https://..." } with no auth. That payload validates before and after this PR, and §9 now calls auth "optional".
This PR shouldn't try to fix that, because a required auth rejects payloads that are valid today, and the issue says that needs a spec version change. So the right line today is Part of #51. Fixes #51 closes the issue when this merges.
@bar-capsule @afogel, this PR adds normative text that no lead has accepted. It follows Carlos Hernandez's 2026-09-13 proposal on issue #51, which has no lead answer. The new text is in §9 (the human approver sentence and a MUST NOT), §9.1 ("when and only when a verified approver grants", and a MUST NOT), and the timeout_disposition description in ask-details.json. I need four answers:
-
Which issue #51 option do you want? I said option 3 looked right and left the call to you. This PR is option 1. Under option 3 it is
Part of, and the issue stays open for a schema change in a later spec version. Under option 1 alone,Fixescan stay. Either way, how does aserviceapprover with noauthmeet "REQUIRED"? §9 answers that for humans only, and Carlos asked whether a preconfigured channel counts. -
Who must follow the two timeout MUST NOTs, and where does the timeout outcome go? The text says a timeout "records" an outcome, and it names no actor and no place. In the reference host, the Observed Agent resolves the timeout and the Guardian never hears about it. I ran the real
governStepon an expired ASK. The step ran asallowwith reason codeask_expired, 0 audit events, and no second Guardian request.ContextEntryis kept by the Guardian, so a Guardian rule can't see this timeout. -
Can a timeout
allowrun a request outsideIntent.parsed? This PR stops the timeout from adding capabilities, but the action still runs (same probe). §8.4 says the capability set "can grow only through explicit, audited approver action". Carlos's proposal said a timeout "cannot supply missing authority", and this PR left that out.The words "no verified approver decision" have the same problem. As I read them, a refusal with a bad token counts as no answer, and
allowthen runs the action. I couldn't run that case, because the spec has no path for the answer. -
Can this text land before issue #175? §9 says the deployment "conveys the verified identity to the Guardian". Issue #175 says the spec has no path for the approver's answer, and it is still in triage. On that issue, Konstantin Trunin and Imran Siddique suggest recording an unsigned answer as declared or agent-asserted. Does "a claimed
approver.idalone is insufficient" rule that out?Carlos wrote that a deployment "can" do this, and this PR says it "authenticates", which reads as a requirement.
Once you answer, Raghuvar can push and one of you can approve and merge.
Raghuvar, please wait for that answer, so you only push once. The words in §9 and §9.1 may change. I applied these four asks to a scratch copy. pytest and mkdocs build --strict pass, and PR #339 merges clean on either side.
-
In the PR body, under "Which issue does this implement", change
Fixes #51toPart of #51, unless the leads pick option 1 alone. Theclosing-choicecode frommainpasses that line. -
docs/spec/instrument/specification.md:317needs two small changes.STYLE.mdasks for 2 to 4 sentences in a paragraph, and this one has six, so start a new paragraph at "Approver authentication is REQUIRED". Also,authis an object, andbearerandmtlsare values ofauth.method, so"auth": "bearer"fails the schema. Change "The optionalapprover.authmember (bearerormtls)" to "The optionalapprover.authobject (method:bearerormtls)". -
specification/v0.1.0/ask-details.json:43repeats the rule from §9 and §9.1, so the spec has it three times.SPEC_REVIEW_PRINCIPLES.mdsection 1 asks us to state a rule in one place. Change the description to:Disposition applied when timeout_seconds elapses without a verified approver decision. See Specification §9 and §9.1. -
Your test at
tests/test_spec_structure.py:57has two problems. Its docstring says it checks the human approver rule, but nothing does. I tried 15 breaks and 12 passed, including deleting the human sentence and changing either MUST NOT to MAY. It also conflicts with PR #339 in both orders. The two tests share lines, so if someone only deletes the conflict markers, PR #339's test has 0 asserts and the suite stays green at 925 passed.Moving the test fixes both problems. Delete it, and add this file as
tests/test_approver_model.py. All 15 breaks then fail, and this PR no longer has to wait for PR #339. If the leads change the words, change these strings to match."""Approver rules in Specification §9 and ask-details.json (#51).""" import json from pathlib import Path ROOT = Path(__file__).resolve().parent.parent SPEC = ROOT / "docs" / "spec" / "instrument" / "specification.md" ASK = ROOT / "specification" / "v0.1.0" / "ask-details.json" def approver_section(): text = SPEC.read_text(encoding="utf-8") return text.split("## 9. Approver Model", 1)[1].split("## 10.", 1)[0] def test_every_approver_type_must_authenticate(): section = approver_section() assert "REQUIRED across all three approver types" in section assert "the Guardian MUST verify" in section assert "out of band" in section assert "a claimed `approver.id` alone is insufficient" in section assert "does not by itself establish that verification occurred" in section def test_a_timeout_is_never_recorded_as_an_approval(): section = approver_section() assert 'default `"deny"`' in section assert "it MUST NOT record the named" in section assert "it MUST NOT append `intent_extension` capabilities" in section assert "when and only when a verified approver grants" in section def test_the_v0_1_0_wire_contract_is_unchanged(): """A required `auth` breaks valid payloads, so issue #51 keeps it for a later spec version.""" schema = json.loads(ASK.read_text(encoding="utf-8")) assert schema["properties"]["approver"]["required"] == ["type", "id"] assert schema["properties"]["timeout_disposition"]["default"] == "deny" assert "§9" in schema["properties"]["timeout_disposition"]["description"]
|
@rocklambros @drk-ram, answers to the four: 1: Option 3. This PR is the text now, and requiring auth waits for the version bump, so Part of #51 stays. Whether a preconfigured authenticated channel counts for a service approver with no auth is an identity question, so I'd hand it to the Identity workstream (@RbBuiltWrong). 2+3: I see ASK as a soft deny: "I won't do this unless someone with authority says yes." So the Guardian holds the timeout, and no answer in time means deny. If the Guardian can't be reached at that point, it's an ordinary decision failure (#32). The Observed Agent never turns an ASK into a go on its own. That leaves timeout_disposition: allow with no meaning, so it goes away with the #32 PR's version bump, and a timeout can't add authority because it can't allow anything. An answer that fails verification blocks, as any integrity failure does (#32). 4: Yes, the authentication text can land first. "A claimed approver.id alone is insufficient" stays: an answer that only the agent vouches for can be recorded as declared, as proposed on #175, but it doesn't approve the step. Approval needs a verified approver: through the deployment's trusted channel today, or the approver's signature once #175 defines it. Rock, that also names the timeout owner you asked for on #175: the Guardian. @drk-ram, since the timeout rules now come with the #32 PR, please drop the timeout paragraph in §9, the timeout sentence in §9.1 and the timeout_disposition description, and keep the authentication text and "when and only when a verified approver grants". Rock's asks 2 and 4 still apply, without the timeout checks in the test; ask 3 goes away with the description. cc: @afogel |
…ervice approvers in §9 Clarifies in §9 and §9.1 of the Instrument specification that approver authentication is required across all three approver types, distinguishes the optional `approver.auth` object (`method`: `bearer` or `mtls`) from out-of-band human approver verification, and requires a verified approver grant before extending `Intent.parsed`. Part of GenAI-Security-Project#51 Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
3df0745 to
86df4e7
Compare
|
@bar-capsule @rocklambros Rebased onto
|
What changed
Clarifies in
docs/spec/instrument/specification.md(§9 and §9.1) that approver authentication is required acrosshuman,agent, andserviceapprovers (with the optionalapprover.authobject describing machine-approver authentication and human approver identity verified out of band against policy), requires a verified approver grant in §9.1 before extendingIntent.parsed, and addstests/test_approver_model.py.Which issue does this implement
Part of #51
meaning, so it needs no issue
Base branch
integration, because this touches the specification, schemas, a referenceimplementation, an adapter, tests, or CI
main, because every changed path is on the documentation lane allowlist inCONTRIBUTING.md
Type of change
Specification changes
Breaking for implementers? No. Keeps
ask-details.jsonwire validation rules unchanged in v0.1.0 while clarifying the separation betweenapprover.authdescriptors and out-of-band human approver verification.I tested this
uv run pytest -vpasses on my machineuv run mkdocs build --strictpasses on my machineChecklist
git commit -s(required by the DCO)Security
Clarifies that a claimed
approver.idalone is insufficient and thatIntent.parsedextensions onscope: sessionapply when and only when a verified approver grants the request.