docs: add external checkpoint bridge example - #3342
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Welcome to the Agent Governance Toolkit! Thanks for your first pull request. |
github-actions
Bot
added
the
documentation
🤖 AI Agent: contributor-guide — View details
Welcome, and thank you for contributing! Your example is well-documented and demonstrates thoughtful design. Before merging:
For guidance, see CONTRIBUTING.md. |
🤖 AI Agent: breaking-change-detector — API Compatibility
API CompatibilityNo breaking changes detected. |
🤖 AI Agent: security-scanner — View details
No security issues found. |
🤖 AI Agent: code-reviewer — View details
TL;DR: 0 blockers, 1 warning. Example implementation is solid but lacks automated tests for critical paths.
Action items: Add unit tests for remote checkpoint validation logic, especially for malformed responses and enforcement mapping. Warnings: Fine as follow-up PRs. |
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs SyncDocumentation is in sync. |
🤖 AI Agent: test-generator — `examples/external-checkpoint-bridge/demo.py`
|
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- remote_checkpoint() calls urllib.request.urlopen on EXTERNAL_CHECKPOINT_URL with no scheme check; README says "HTTPS endpoint" but http:// and file:// are accepted. Add
if not url.startswith("https://"): raise ValueError(...). This is a security-pattern example in a governance repo and should model transport hygiene, not just hash binding.
Imran Siddique (imran-siddique)
left a comment
Imran Siddique (imran-siddique)
left a comment
There was a problem hiding this comment.
Self-contained interop example, correctly scoped and fail-safe (deny/pause mapping, action_hash rebinding check). Consider validating the https scheme on EXTERNAL_CHECKPOINT_URL to match the README.
|
Thanks for the review. Updated in 42f020f to validate EXTERNAL_CHECKPOINT_URL before any remote checkpoint call: the demo now requires an https scheme and a non-empty host, so http://, file://, and malformed https URLs fail closed before urllib.request.urlopen is reached. I also added focused tests for the URL validation path and the existing action_hash rebinding check. Local verification run:
|
|
MohammadHaroonAbuomar thanks again for the security-pattern review. I addressed the transport hygiene point in the latest update: I also added focused tests for the URL validation path and the existing Local verification I ran:
Would appreciate a re-review when you have a chance. |
42f020f to
ea8ab56
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The demo’s action_ref derivation and remote checkpoint parsing need tightening to avoid sensitive-data leakage and avoid unclear runtime failures from malformed/invalid remote responses.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds a new dependency-free example under examples/ that demonstrates sending an AGT-style action envelope to an optional external checkpoint/verifier (via EXTERNAL_CHECKPOINT_URL) and mapping its verdict back into local enforcement semantics.
TL;DR: 2 blockers, 2 warnings. Fix action_ref derivation and remote response validation and this ships.
| # | Sev | Issue | Where |
|---|---|---|---|
| 1 | Block | action_ref is not hashed and can leak sensitive args; also contradicts “hashing” claim |
demo.py |
| 2 | Block | Remote payload assumptions can raise AttributeError/KeyError instead of a clear error |
demo.py |
| 3 | Warn | Deterministic JSON serialization should be tightened for canonicalization consistency | demo.py |
| 4 | Warn | Add tests for action_ref determinism + invalid remote verdict handling (fine as follow-up) |
test_demo.py |
Changes:
- Adds a runnable Python demo implementing local vs. remote checkpoint review and verdict→enforcement mapping.
- Adds a README walkthrough describing usage, expected output, and scope.
- Adds pytest coverage for HTTPS URL validation and action_ref mismatch rejection.
File summaries
| File | Description |
|---|---|
| examples/external-checkpoint-bridge/demo.py | Implements deterministic envelope creation, optional remote checkpoint call, and verdict→enforcement mapping demo. |
| examples/external-checkpoint-bridge/README.md | Documents how to run the example locally or against an HTTPS checkpoint endpoint. |
| examples/external-checkpoint-bridge/test_demo.py | Adds basic pytest coverage for remote URL validation and action_ref mismatch handling. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| def stable_json(value: Any) -> str: | ||
| """Serialize JSON deterministically for hashing and checkpoint review.""" | ||
| return json.dumps(value, sort_keys=True, separators=(",", ":")) |
| ref_input = { | ||
| "actor": actor, | ||
| "runtime": runtime, | ||
| "tool_name": tool_name, | ||
| "proposed_action": proposed_action, | ||
| "arguments": arguments, | ||
| "policy_id": policy_id, | ||
| } | ||
| return { | ||
| "action_ref": stable_json(ref_input), | ||
| **ref_input, | ||
| } |
| with urllib.request.urlopen(request, timeout=10) as response: | ||
| payload = response.read().decode("utf-8") | ||
| verdict = json.loads(payload) | ||
|
|
||
| if verdict.get("action_ref") != envelope["action_ref"]: | ||
| raise ValueError( | ||
| "Remote checkpoint returned a verdict for a different action_ref." | ||
| ) | ||
|
|
||
| return { | ||
| "verdict": verdict["verdict"], | ||
| "reason": verdict.get("reason", "External checkpoint returned no reason."), | ||
| "decision_id": verdict.get( | ||
| "decision_id", f"remote-{envelope['tool_name']}" | ||
| ), | ||
| "action_ref": envelope["action_ref"], | ||
| } |
| assert observed == { | ||
| "url": "https://checkpoint.example.com/review", | ||
| "timeout": 10, | ||
| } |
github-actions
Bot
added
size/XL
|
Addressed the latest Copilot feedback in 5e99cda. Changes made:
Verification run locally:
The latest remote checks are also green. The remaining AI code-review output is a warning only and lists no action items. |
There was a problem hiding this comment.
🟡 Not ready to approve
parse_remote_verdict() can currently throw TypeError on malformed remote payloads, which breaks the intended fail-closed error handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
examples/external-checkpoint-bridge/test_demo.py:127
- Add a regression case where
verdictis a non-string (e.g.,[]) to ensure malformed remote payloads produce aValueError(and don’t regress back toTypeErrorfrom set membership checks).
[
("not-json", "invalid JSON"),
("[]", "JSON object"),
(
json.dumps(
{
"verdict": "escalate",
"reason": "Unsupported verdict.",
"decision_id": "dec_test",
"action_ref": "use-envelope-ref",
}
),
"verdict must be one of",
),
examples/external-checkpoint-bridge/demo.py:165
parse_remote_verdict()can raise aTypeErrorinstead of the intendedValueErrorwhen the remote payload contains a non-stringverdict(e.g., a JSON array), because set membership onALLOWED_VERDICTSrequires a hashable value. This undermines the “fail closed with a clear error” goal.
verdict = raw_verdict.get("verdict")
if verdict not in ALLOWED_VERDICTS:
raise ValueError(
"Remote checkpoint verdict must be one of: allow, require_approval, deny."
)
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The change is isolated to a self-contained example plus tests, with only minor best-practice follow-ups identified.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
tests/test_external_checkpoint_bridge.py:23
- The dynamic import should assert that the module spec and loader are not None (matching existing test patterns in tests/ci/*). Without these asserts, a missing/invalid path would fail with less-informative AttributeError/TypeError at import time.
_spec = importlib.util.spec_from_file_location(
"external_checkpoint_bridge_demo", _EXAMPLE_DIR / "demo.py"
)
demo = importlib.util.module_from_spec(_spec) # type: ignore[arg-type]
sys.modules["external_checkpoint_bridge_demo"] = demo
examples/external-checkpoint-bridge/demo.py:131
- Use the standard "Content-Type" header casing for the JSON payload. While header names are case-insensitive per HTTP, some intermediary tooling/server frameworks are buggy and may not recognize non-standard casing.
headers={"content-type": "application/json"},
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
Only a minor test-loading convention issue was found; the example logic and validation behavior are well-covered by the added tests.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
tests/test_external_checkpoint_bridge.py:23
- The dynamic module load should assert the spec/loader are present (as done in other tests under
tests/ci/) instead of relying ontype: ignore; this avoids obscureAttributeErrorfailures during pytest collection if the example path changes or the loader is unavailable.
_spec = importlib.util.spec_from_file_location(
"external_checkpoint_bridge_demo", _EXAMPLE_DIR / "demo.py"
)
demo = importlib.util.module_from_spec(_spec) # type: ignore[arg-type]
sys.modules["external_checkpoint_bridge_demo"] = demo
_spec.loader.exec_module(demo) # type: ignore[union-attr]
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The change is self-contained (new example + tests), follows existing repo conventions, and introduces no risky behavior into production code paths.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Update on the previous review items:
Local verification run:
MohammadHaroonAbuomar the transport-hygiene concern from your changes-requested review should be addressed now; a re-review would unblock the stale review state. |
8809b65 to
156a597
Compare
Signed-off-by: jw_ond <THU-Tokyo@outlook.com>
Signed-off-by: jw_ond <THU-Tokyo@outlook.com>
Signed-off-by: jw_ond <THU-Tokyo@outlook.com>
Signed-off-by: jw_ond <THU-Tokyo@outlook.com>
Signed-off-by: jw_ond <THU-Tokyo@outlook.com>
Signed-off-by: jw_ond <THU-Tokyo@outlook.com>
Signed-off-by: jw_ond <THU-Tokyo@outlook.com>
Signed-off-by: jw_ond <THU-Tokyo@outlook.com>
156a597 to
71e709c
Compare
|
Rebased this branch onto current main to remove the stale-base CI noise that was affecting the spell/link jobs. What changed in this update:
Local verification:
I also checked #3620 and #3613. #3620 explains the previous markdown-link-check failure mode: the job was checking files changed on main since the branch point, not only files changed by this PR. #3613 references this PR as an external-checkpoint/verifier example, but it is a separate MAF execution-identity RFC rather than a duplicate of this example. |
Description
Adds a dependency-free
examples/external-checkpoint-bridgewalkthrough showing how an AGT-style action envelope can be sent to an external checkpoint or verifier before tool execution, then mapped back to local AGT enforcement semantics.The example demonstrates:
EXTERNAL_CHECKPOINT_URLallow,require_approval, anddenyverdict mappingThis keeps AGT as the runtime enforcement point while allowing an external verifier, ledger, or review service to contribute an independent signal.
Type of Change
Package(s) Affected
Checklist
Attribution & Prior Art
Prior art / related projects (if any):
External governance checkpoints and verifier services that review proposed actions before runtime execution. The example is intentionally generic and does not depend on a specific external service.
AI Assistance
If AI tools materially shaped this change, briefly note what was used:
Codex helped draft the example and README; output was reviewed before submission.
IP, Patents, and Licensing
Related Issues
N/A
Verification
/tmp/openai-agents-python-ruff/bin/ruff format examples/external-checkpoint-bridge/demo.py/tmp/openai-agents-python-ruff/bin/ruff check examples/external-checkpoint-bridge/demo.py/opt/homebrew/bin/python3.11 -m py_compile examples/external-checkpoint-bridge/demo.py/opt/homebrew/bin/python3.11 examples/external-checkpoint-bridge/demo.py