feat(agw): add implicit auditlog usage on agw#209
Conversation
ricardosrib
changed the title
a9e31e7 to
5515bea
Compare
BSD awk (macOS) requires the '/' inside a character class to be escaped as '\/'. GNU awk accepts either form. Discovered during live run on PR #209. Impact: check-testing-depth now returns valid JSON on macOS orchestrate runs; previously it silently emitted an awk error and aggregate.sh skipped the report.
Discovered during live run against PR #209. Multiple checks used the pattern: count=$(echo "$diff" | grep -E 'pattern' 2>/dev/null | wc -l | tr -d ' ') Under 'set -o pipefail', when grep finds zero matches it returns exit 1, which propagates through the pipe. Under 'set -e', the containing script exits silently — no stderr, no stdout, exit code 1. The 'aggregate.sh skips invalid JSON' safety net (added earlier) masked this by producing a summary that omitted the failing checks. Fix: wrap each grep-in-pipe with { grep … || true; } so zero-match returns success. Affects: - check-deps-supply.sh (count_lines helper) - check-pr-size.sh (count_lines helper + mods_count) - check-telemetry.sh (client_files, test_files, new_decorators) - check-testing-depth.sh (new_modules) - check-deletion-hygiene.sh (hits) Impact: live orchestrate.sh runs now emit valid JSON for all 20 checks instead of 15/20. No FP regression (all fixes preserve semantics). Discovered by: real dry-run on cloud-sdk-python PR #209 (Ricardo's agentgateway auditlog PR).
Discovered live on cloud-sdk-python PR #209 (Ricardo, agentgateway auditlog). The check was counting string occurrences from the on-disk AST — global to the file — then anchoring the finding to the first occurrence's line number. When a PR (a) shifts line numbers by adding upstream code and (b) does not touch any of the actual repeated-string lines, PY-CON-01 fires for tech debt that pre-existed the PR. Fix: read $ADDED_LINES_FILE (already exported by orchestrate.sh) inside check_con_01. Filter each literal's occurrence list to lines actually in the PR's added set. Only fire when ${filtered} is non-empty AND total occurrences (across file, unchanged) still >= threshold. Anchor line becomes the first added-line occurrence. Backward-compat: if ADDED_LINES_FILE is unset (bats tests exercising the lib directly), fall back to the legacy full-file behavior. Two new regression tests pin both branches (untouched pre-existing repetition = no fire; at least one occurrence on added line = fires with correct anchor). Bats: 81/81 green on both repos.
tiagoek
added
sdk-review: ❌ blocked
sdk-review: ⚠️ flagged
and removed
sdk-review: ❌ blocked
sdk-review: ⚠️ flagged
labels
|
Update user guide |
a2a5c6e to
867afd5
Compare
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| MCP_TOOL_INVOKED = "MCP_TOOL_INVOKED" |
There was a problem hiding this comment.
Isn't better to have enum for it?
There was a problem hiding this comment.
I think an enum would add no much value here because, differently from AuditLogMode where each value means a different behavior for implicit audit logging, these string constants are just labels — they have no meaning as a structured type event. Also, an enum would require .value at every call, adding noise for no benefit in this case.
| MCP_TOOL_FAILED = "MCP_TOOL_FAILED" | ||
|
|
||
|
|
||
| def create_audit_client( |
There was a problem hiding this comment.
Could we have it as a helper for internal usage and not specific to agw?
There was a problem hiding this comment.
Created helper.py in core/auditlog_ng/ with the reusable parts: AuditLogMode, create_audit_client, and send_event.
Any module that wants implicit audit logging only needs to:
- Call create_audit_client at client construction time
- Declare AuditLogMode in its ClientConfig
- Define its own event name constants and thin wrappers on top of send_event
The AGW-specific implicit_auditlog.py is now just the MCP_TOOL* constants and three public functions. The protobuf construction and mode/tenant handling are fully abstracted in core.
The same pattern can be reused by any module that wants to add implicit audit logging.
| "send_audit_event_failed", | ||
| ] | ||
|
|
||
| MCP_TOOL_INVOKED = "MCP_TOOL_INVOKED" |
There was a problem hiding this comment.
I believe this file can be removed and we can have an enum for event and direct use the send_event. What do you think?
There was a problem hiding this comment.
We could even consider a decorator for it
| for tenant-scoped token exchange. | ||
| TODO: This parameter's requirement is still being clarified with | ||
| the IBD team and may be removed if unnecessary. | ||
| user_id: User identifier recorded in the audit event when an |
There was a problem hiding this comment.
We should retrieve it from user token instead of asking. You can check IAS module for it.
|
|
||
| --- | ||
|
|
||
| ## Sending Custom Events (ZzzCustomEvent) |
There was a problem hiding this comment.
Couldn't each module define events?
| @@ -0,0 +1,148 @@ | |||
| """Audit log helper for custom events. | |||
There was a problem hiding this comment.
Maybe we can rename it to refer this is for implicit usage of auditlog cross modules
3 participants
Description
Implicit audit log emission in AgentGatewayClient - the client now automatically creates an AuditClient on initialization (using the LoB destination) and sends a DataAccess audit event after every successful list_mcp_tools() and call_mcp_tool() call. The tenant ID is read from the telemetry context var (get_tenant_id()).
Related Issue
N/A
Type of Change
Please check the relevant option:
How to Test
Checklist
Before submitting your PR, please review and check the following:
Breaking Changes
N/A
Additional Notes
N/A