Repository navigation
fix(guardian): read failure event and step attributes from otel-mapping.json and harden audit-to-otlp - #349
Conversation
|
This pull request's description says it closes #284, 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. |
6e02a45 to
33853d9
Compare
24a098a to
4349625
Compare
rocklambros
left a comment
There was a problem hiding this comment.
Raghuvar, your converter checks work. I deleted lines from a real audit log and ran the command. Integration exits 0 and says nothing, and your branch exits 1. Prototype names like toString now map to other, and a 21-digit error code is clamped.
Your 4 tests fail on integration and pass on your branch. Every suite passes on integration 19b1197: pytest 923 passed and 1 skipped, and 1290 Bun tests with 0 failures on 1.3.11 and 1.3.14.
This PR doesn't close issue #284 yet. The issue says each item needs a test that fails before the change and passes after it.
Item 2 is done. Item 4 works, but parts of it have no test. Items 1 and 5 work and need the changes below.
Item 3 has the eight names, written by hand, with no test against the mapping. Item 7 is not done. Five of the six test items in item 6 are still open. The sixth needs no change, because a wrong acs.exit_status field already fails test/sample-trace.test.ts:224.
Saba Siddique claimed issue #284 first, on 2026-10-15, and he has no branch or PR for it. I saw your comment there, and I asked him whether he's fine with you finishing it: #284 (comment). Please wait for his answer before you push, so you don't do work that he also does. You don't need to wait for PR #229.
Thanks for accepting the invite. Your other waiting issues are now assigned to you, and this one stays unassigned until he answers.
@bar-capsule, you're the reviewer on this issue, and ask 1 has one fact for you. Hooks that run at the same time can write the same seq. I ran 8 hooks at once, 6 times. No line was lost, and every run exited 1.
Issue #284 asks for a non-zero exit on duplicates, so ask 1 keeps that. If you'd rather have duplicates print a line and exit 0, tell Raghuvar. That is one condition on the last if of the command.
Paths start at reference-implementations/agt/, and line numbers are from your current head. I ran all of these asks in a scratch copy, on integration and on top of PR #229. On integration, 1314 Bun tests pass on 1.3.11, 1.3.14 and 1.4.4. On top of PR #229, the only failure is the Bun 1.3.14 test that Jason Crittenden already has to fix.
-
Item 5,
packages/guardian/src/audit-to-otlp.ts:246and:262-268. Your check can't tell a lost line from hooks that ran together. The host writesseqas one more than the larger of the highestseqand the line count (packages/host-adapter/src/audit-sink.ts:281). A healthy run gave[1, 1, 3, 1, 1, 4, 6, 6]. No line was lost there, and your code counts the 3 as a gap. Lines with no number also skip your check: I removedseqfrom every line and deleted one line, and the command exited 0. Use this rule:// before the loop const sequence = { gaps: 0, duplicates: 0, backwards: 0, unnumbered: 0 }; const seenSeq = new Set<number>(); let highestSeq = 0; let lineCount = 0; // after the blank-line check, before JSON.parse. A half-written line counts, because the host counts it too. const linesBefore = lineCount; lineCount += 1; // in place of your seq block const seq = entry.seq; if (typeof seq !== "number" || !Number.isInteger(seq) || seq < 1) { sequence.unnumbered += 1; } else { if (seenSeq.has(seq)) { sequence.duplicates += 1; } else if (seq > Math.max(highestSeq, linesBefore) + 1) { sequence.gaps += 1; } seenSeq.add(seq); highestSeq = Math.max(highestSeq, seq); }
Count
recorded_atgoing backwards insequence.backwards. InConversionSummary, replacesequenceDiscrepancieswithsequence, and set it only when a count is above zero. Remove theasNumberimport, because nothing uses it now. At:383-386, print the line below to stderr before thewrittencheck at:379, so both reports can appear, and exit 1 if either one fired.audit log order: 1 gap(s), 0 duplicate(s), 0 recorded_at going backwards, 0 line(s) with no usable seqTest: your test never runs the command, so deleting the exit passes every test. In
test/audit-to-otlp.test.ts, run the command withBun.spawn, and check the exit status and that line.[1, 2, 4, 5]and[2, 3, 4]give 1 gap.[1, 2, 2, 3]gives 1 duplicate.[1, 1, 3, 1, 1, 4, 6, 6]gives 0 gaps and 4 duplicates. A missingseq,"1"and2.5count as no usable seq. A half-written line betweenseq1 and 3 exits 0, and so does[1, 2, 3]. -
Item 1,
packages/guardian/src/otel-mapping.ts:217-221and:224. The fallbacks mean the code still works if it never reads the mapping. I replaced each read with a fixed value, and all 1290 tests still passed. Read both the waydecision_eventis read, with no?.and no??, and remove the?at:168and:180. DeleteDECISION_FAILURE_EVENT(:41and its exports). Atpackages/guardian/src/otel-trace-exporter.ts:544and:651, usemapping!.failureEventName, because a step span exists only after the mapping loaded. Delete the text atotel-mapping.ts:3and:312-318, because both still say the mapping lacks these names.packages/guardian/test/otel-trace-exporter.test.ts:577builds a small mapping, so add the two keys there.Test: replace the test at
otel-trace-exporter.test.ts:1826. Copy the mapping, rename the event toacs.renamed.failure, and setstep_span_attributesto["acs.session.id"]. Check the new name on all five paths: an error, a late decision, aguardian_fault, a step open at shutdown, and the converter. Check thatacs.request_idis gone from the step spans. -
otel-trace-exporter.ts:475.claimedalso feeds the rejected span. If the mapping dropsacs.session.id, a rejected request loses its claimed session id, and integration keeps it. Issue #283 needs those ids on a forged request. KeepObject.keys(CORRELATION_ATTRIBUTE_SOURCES)here, and add this helper for:564and:649:function stepIds(stepSpan: StepSpan, stepAttributes: readonly string[]): Attributes { return Object.fromEntries(Object.entries(stepSpan.claimed).filter(([name]) => stepAttributes.includes(name))); }
Test: in ask 2's test, check that the
guardian_faultspan still hasguardian.rejected.claimed_request_id. -
Items 3 and 7,
otel-mapping.ts:101. A TypeScript type can't read a JSON file, so keep the names in a list and test the list against the mapping. For item 7,audit-to-otlp.ts:342writes the detail as the status, so arefusedentry and the Guardian's own error have different sentences, and both areguardian_error. Add one table for both writers:export const FAILURE_CLASSES = ["timeout", "unreachable", "malformed_response", "guardian_error", "observed_agent_error", "signature_invalid", "handshake_failure", "other"] as const; export type FailureClass = (typeof FAILURE_CLASSES)[number]; export const FAILURE_STATUS: Readonly<Record<FailureClass, string>> = { timeout: "No decision arrived before the host's timeout.", unreachable: "The host could not reach the Guardian.", malformed_response: "An answer arrived that could not be used.", guardian_error: GUARDIAN_ERROR_STATUS, observed_agent_error: "The host could not build a request for this hook.", signature_invalid: "A reply arrived that failed verification.", handshake_failure: "The handshake did not complete.", other: "The step ended without a usable decision.", };
Use
FAILURE_STATUS[failureClassOf(kind)]ataudit-to-otlp.ts:342,FAILURE_STATUS.guardian_erroratotel-trace-exporter.ts:572, andFAILURE_STATUS.otherat:654. Keepacs.failure.detailas it is, because that's where the kind stays visible.Test:
[...FAILURE_CLASSES].sort()equals the sortedacs.failure.classlist inotel-mapping.json. For every key ofFAILURE_DETAIL_BY_KINDplusunknown, the converter's status equalsFAILURE_STATUS[failureClassOf(kind)]. Check the exporter's error span and its shutdown span too. -
Item 4,
otel-mapping.ts:353. A 400-digit code parses to-Infinity,asNumberrefuses it, and the code disappears.Math.truncturns-32005.5into-32005, and the exporter then records a rejected request. Integration keeps that one on the step. Use this:export function asClampedErrorCode(value: unknown): number | undefined { if (typeof value !== "number" || Number.isNaN(value)) return undefined; return Math.max(Number.MIN_SAFE_INTEGER, Math.min(Number.MAX_SAFE_INTEGER, value)); }
Test:
-1e21,1e21and 400 digits clamp to the exact bound,-32005.5stays the same, and an exporter span with-1e21carries the bound. Your test checks onlyfailureClassOf, so removingObject.hasOwnataudit-to-otlp.ts:294or:312still passes. Convert atoStringkind with a method and one withmethod: null, then check the detail sentence andskippedNoMethodequal to{ other: 1 }. -
Item 6, the tests that are missing. In
packages/guardian/test/otel-trace-exporter.test.ts:- Empty array: send
{ ...DENY, reason_codes: [], policy_references: [] }, and check thatacs.decisionis there andacs.reason_codesand the threeacs.policy.*names are not. - Off by default,
:1135: the Guardian never getspath, so this test can't fail. Check thatcreateOtelTraceExporter({ methodsEvaluated: METHODS_EVALUATED })isNULL_OTEL_TRACE_EXPORTER. Thenprocess.chdir(dir)beforestartGuardian, withmanifestPathresolved first, and check thatreaddirSync(dir)is empty at the end. - Your test at
:1411-1412passes if no event exists. Checkacs.decisionisdenyfirst, then loop over all six extras.
In
test/audit-to-otlp.test.ts:- Join: start a real Guardian with
otelTraceFile, rungovernStepwithcreateGuardianClient(guardian.url), and convert the audit log. Check that the two spans have the sameacs.session.idandacs.request_id. - Posture:
posture: "deny"withoutcome: "proceeded"givesfail_closed."maybe"in either field counts as unreadable. - Parsing: both host message shapes give a code, and
a proxy said error -7: xgives none.not json,[1],42andnullcount as unreadable. A badrecorded_attimes the span at now. - Slow test,
:243: add30_000as the third argument toit. It took 0.4 s here and about 19 s for Bar, so this one can have no fail-before test.
- Empty array: send
-
packages/guardian/README.md:121still says the command exits non-zero only when spans are missing. Change it to say that it also exits non-zero when the log's order is wrong. Add a bullet after:141with the new line. Say that the output is still complete for the lines that remain, a gap means a line is missing, and duplicates or timestamps going backwards can come from hooks that ran at the same time. Say that deleting the last lines can't be detected, because nothing comes after them to compare. Put the same facts in the header ofaudit-to-otlp.ts.
If PR #229 merges first, otel-mapping.ts has one conflict at the FailureClass block. Keep your FAILURE_CLASSES list and table. The test in ask 4 fails if a name goes missing. PR #229 also adds signature_invalid rows to the two tables in audit-to-otlp.ts, and those merge clean.
Issue #284 closes once these changes are in. If Saba keeps part of the issue, change Fixes #284 to Part of #284.
…ng.json and harden audit-to-otlp Read decision_failure_event and step_span_attributes directly from specification/v0.1.0/trace/otel-mapping.json without fallbacks, export FAILURE_CLASSES and FAILURE_STATUS for shared span status sentences across both writers, filter step span attributes via stepIds while retaining claimed ids on rejected spans, clamp oversized error codes without truncating fractional codes, and track gaps, duplicates, backwards timestamps, and unnumbered lines in audit-to-otlp.ts. Part of GenAI-Security-Project#284 Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
4349625 to
6323e34
Compare
|
@rocklambros Pushed
|
Make the audit converter exit non-zero for a line that does not parse and for an entry with no usable recorded_at, and count only safe integers as seq. Escape and bound ids nothing has verified. Refuse a mapping at load when an event name is missing or shared, or when step_span_attributes lists a name with no source. Both writers now read the join ids from one table through collect(). Read every error code the host can write as String(code), so -1e+21, -Infinity and -32005.5 keep acs.failure.error_code as they do in the exporter. Give a late decision the timeout status and a guardian_fault span the guardian_error status. Build the converter's detail sentences from FAILURE_STATUS. Regenerate the sample trace for the new status. Keep one status loop, with literal sentences, and add tests for the mutants that survived. Remove unused imports, an unused alias and unread types, correct stale comments, and update the README and header comment for the exit behavior. Part of GenAI-Security-Project#284 Signed-off-by: rocklambros <rock@rockcyber.com>
|
Raghuvar, I checked every ask you pushed in What changed:
Every suite passes on the merged tree, except Two things are still open, and neither is yours. Saba has item 6 of issue #284, so this PR stays I pushed last, so my approval doesn't count here. @bar-capsule @afogel, one of you needs to approve this one. |
All asks are done or moved to Saba Siddique on issue #284, and my fixes are in 137c773. Dismissing so this does not block the merge.
|
Reviewed the whole PR, including One change, answering Rock's question: duplicates and
#284 item 5 should say the same, and I'll note it there. Also, please add |
…udit-to-otlp Print the order line on stderr and exit 0 when the only order findings are duplicate `seq` values or `recorded_at` timestamps going backwards, while keeping exit 1 for `seq` gaps, lines with no usable `seq` (including unparseable lines), entries with no usable `recorded_at`, and missing spans. Part of GenAI-Security-Project#284 Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.users.noreply.github.com>
|
@bar-capsule @rocklambros Pushed
|
What changed
Reads
decision_failure_event(event_name) andstep_span_attributes(required_attributes) directly fromspecification/v0.1.0/trace/otel-mapping.jsoninpackages/guardian/src/otel-mapping.tswithout fallbacks, removesDECISION_FAILURE_EVENTandDECISION_DETAIL_NAMES, filters step span attributes viastepIdswhile keeping claimed correlation IDs on rejected spans, exportsFAILURE_CLASSESandFAILURE_STATUSfor shared span status sentences across both writers, clamps oversized error codes without truncating fractional codes, and hardenspackages/guardian/src/audit-to-otlp.ts:recorded_atexit 1, while out-of-range or unusable timestamps are timed at now.seqvalues andrecorded_atgoing backwards print the order line on stderr and exit 0, whileseqgaps, lines with no usableseq, entries with no usablerecorded_at, and missing spans exit 1.Which issue does this implement
Part of #284
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
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