Skip to content

fix(guardian): read failure event and step attributes from otel-mapping.json and harden audit-to-otlp - #349

Open
drk-ram wants to merge 3 commits into
GenAI-Security-Project:integrationfrom
drk-ram:refimpl/trace-mapping-names-284
Open

drk-ram wants to merge 3 commits into
GenAI-Security-Project:integrationfrom
drk-ram:refimpl/trace-mapping-names-284

Conversation

@drk-ram

@drk-ram drk-ram commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

What changed

Reads decision_failure_event (event_name) and step_span_attributes (required_attributes) directly from specification/v0.1.0/trace/otel-mapping.json in packages/guardian/src/otel-mapping.ts without fallbacks, removes DECISION_FAILURE_EVENT and DECISION_DETAIL_NAMES, filters step span attributes via stepIds while keeping claimed correlation IDs on rejected spans, exports FAILURE_CLASSES and FAILURE_STATUS for shared span status sentences across both writers, clamps oversized error codes without truncating fractional codes, and hardens packages/guardian/src/audit-to-otlp.ts:

  • Unparseable lines and entries with no usable recorded_at exit 1, while out-of-range or unusable timestamps are timed at now.
  • Duplicate seq values and recorded_at going backwards print the order line on stderr and exit 0, while seq gaps, lines with no usable seq, entries with no usable recorded_at, and missing spans exit 1.
  • Unverified IDs are escaped and bounded in length, both writers read join IDs from one table, and invalid mappings are refused at load time.

Which issue does this implement

Part of #284

  • This is an editorial correction (typo, grammar, link, formatting) with no change in
    meaning, so it needs no issue

Base branch

  • integration, because this touches the specification, schemas, a reference
    implementation, an adapter, tests, or CI
  • main, because every changed path is on the documentation lane allowlist in
    CONTRIBUTING.md

Type of change

  • Specification change (schema, hooks, events, AgBOM)
  • Reference implementation or adapter
  • Documentation
  • Tooling or CI
  • Governance (licensing, security policy, contributor docs)

I tested this

  • I synced my branch with the base branch before opening this
  • uv run pytest -v passes on my machine
  • uv run mkdocs build --strict passes on my machine

Checklist

  • Commits are signed off with git commit -s (required by the DCO)
  • Prose follows STYLE.md
  • No secrets, tokens, or internal URLs in the diff

Security

  • This change has no security impact

@github-actions

Copy link
Copy Markdown

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.

@rocklambros rocklambros 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.

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.

  1. Item 5, packages/guardian/src/audit-to-otlp.ts:246 and :262-268. Your check can't tell a lost line from hooks that ran together. The host writes seq as one more than the larger of the highest seq and 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 removed seq from 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_at going backwards in sequence.backwards. In ConversionSummary, replace sequenceDiscrepancies with sequence, and set it only when a count is above zero. Remove the asNumber import, because nothing uses it now. At :383-386, print the line below to stderr before the written check 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 seq
    

    Test: your test never runs the command, so deleting the exit passes every test. In test/audit-to-otlp.test.ts, run the command with Bun.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 missing seq, "1" and 2.5 count as no usable seq. A half-written line between seq 1 and 3 exits 0, and so does [1, 2, 3].

  2. Item 1, packages/guardian/src/otel-mapping.ts:217-221 and :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 way decision_event is read, with no ?. and no ??, and remove the ? at :168 and :180. Delete DECISION_FAILURE_EVENT (:41 and its exports). At packages/guardian/src/otel-trace-exporter.ts:544 and :651, use mapping!.failureEventName, because a step span exists only after the mapping loaded. Delete the text at otel-mapping.ts:3 and :312-318, because both still say the mapping lacks these names. packages/guardian/test/otel-trace-exporter.test.ts:577 builds 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 to acs.renamed.failure, and set step_span_attributes to ["acs.session.id"]. Check the new name on all five paths: an error, a late decision, a guardian_fault, a step open at shutdown, and the converter. Check that acs.request_id is gone from the step spans.

  3. otel-trace-exporter.ts:475. claimed also feeds the rejected span. If the mapping drops acs.session.id, a rejected request loses its claimed session id, and integration keeps it. Issue #283 needs those ids on a forged request. Keep Object.keys(CORRELATION_ATTRIBUTE_SOURCES) here, and add this helper for :564 and :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_fault span still has guardian.rejected.claimed_request_id.

  4. 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:342 writes the detail as the status, so a refused entry and the Guardian's own error have different sentences, and both are guardian_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)] at audit-to-otlp.ts:342, FAILURE_STATUS.guardian_error at otel-trace-exporter.ts:572, and FAILURE_STATUS.other at :654. Keep acs.failure.detail as it is, because that's where the kind stays visible.

    Test: [...FAILURE_CLASSES].sort() equals the sorted acs.failure.class list in otel-mapping.json. For every key of FAILURE_DETAIL_BY_KIND plus unknown, the converter's status equals FAILURE_STATUS[failureClassOf(kind)]. Check the exporter's error span and its shutdown span too.

  5. Item 4, otel-mapping.ts:353. A 400-digit code parses to -Infinity, asNumber refuses it, and the code disappears. Math.trunc turns -32005.5 into -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, 1e21 and 400 digits clamp to the exact bound, -32005.5 stays the same, and an exporter span with -1e21 carries the bound. Your test checks only failureClassOf, so removing Object.hasOwn at audit-to-otlp.ts:294 or :312 still passes. Convert a toString kind with a method and one with method: null, then check the detail sentence and skippedNoMethod equal to { other: 1 }.

  6. 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 that acs.decision is there and acs.reason_codes and the three acs.policy.* names are not.
    • Off by default, :1135: the Guardian never gets path, so this test can't fail. Check that createOtelTraceExporter({ methodsEvaluated: METHODS_EVALUATED }) is NULL_OTEL_TRACE_EXPORTER. Then process.chdir(dir) before startGuardian, with manifestPath resolved first, and check that readdirSync(dir) is empty at the end.
    • Your test at :1411-1412 passes if no event exists. Check acs.decision is deny first, then loop over all six extras.

    In test/audit-to-otlp.test.ts:

    • Join: start a real Guardian with otelTraceFile, run governStep with createGuardianClient(guardian.url), and convert the audit log. Check that the two spans have the same acs.session.id and acs.request_id.
    • Posture: posture: "deny" with outcome: "proceeded" gives fail_closed. "maybe" in either field counts as unreadable.
    • Parsing: both host message shapes give a code, and a proxy said error -7: x gives none. not json, [1], 42 and null count as unreadable. A bad recorded_at times the span at now.
    • Slow test, :243: add 30_000 as the third argument to it. It took 0.4 s here and about 19 s for Bar, so this one can have no fail-before test.
  7. packages/guardian/README.md:121 still 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 :141 with 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 of audit-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>
@drk-ram
drk-ram force-pushed the refimpl/trace-mapping-names-284 branch from 4349625 to 6323e34 Compare October 31, 2026 23:47
@drk-ram

drk-ram commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator Author

@rocklambros Pushed 6323e34 with asks 1–5, the :1411-1412 update from ask 6, ask 7, and Part of #284 in both the commit message and PR description:

  1. Ask 1 (audit-to-otlp.ts, test/audit-to-otlp.test.ts): Replaced sequenceDiscrepancies with sequence = { gaps, duplicates, backwards, unnumbered }, removed unused asNumber import, printed audit log order: ... to stderr before the written check, and added the Bun.spawn CLI tests covering [1, 2, 4, 5], [2, 3, 4], [1, 2, 2, 3], [1, 1, 3, 1, 1, 4, 6, 6], missing/"1"/2.5 seq, half-written line between seq 1 and 3, and [1, 2, 3].
  2. Ask 2 (otel-mapping.ts, otel-trace-exporter.ts, otel-trace-exporter.test.ts): Removed optional chaining/fallbacks on decision_failure_event and step_span_attributes, removed DECISION_FAILURE_EVENT and obsolete comments, updated the small mapping fixture at :577, and replaced the test at the end of otel-trace-exporter.test.ts to verify acs.renamed.failure and step_span_attributes: ["acs.session.id"] across all five failure paths.
  3. Ask 3 (otel-trace-exporter.ts): Restored Object.keys(CORRELATION_ATTRIBUTE_SOURCES) when populating claimed, added stepIds(stepSpan, stepAttributes) for :564 and :649, and verified guardian.rejected.claimed_request_id on the guardian_fault span.
  4. Ask 4 (otel-mapping.ts, audit-to-otlp.ts, otel-trace-exporter.ts): Added FAILURE_CLASSES and FAILURE_STATUS, used FAILURE_STATUS[failureClassOf(kind)] in the converter and FAILURE_STATUS.guardian_error / FAILURE_STATUS.other in the exporter, and tested FAILURE_CLASSES against otel-mapping.json and FAILURE_STATUS across all failure kinds and both writers.
  5. Ask 5 (otel-mapping.ts, audit-to-otlp.test.ts, otel-trace-exporter.test.ts): Updated asClampedErrorCode to clamp -Infinity/Infinity and preserve fractional numbers like -32005.5, and tested -1e21, 1e21, 400-digit codes, -32005.5, and toString failure kinds with and without a method.
  6. Ask 6 (:1411-1412): Checked acs.decision is "deny" first and looped over all six extras.
  7. Ask 7 (packages/guardian/README.md, audit-to-otlp.ts): Documented the order-check exit condition and troubleshooting message in both places.

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>
@rocklambros

Copy link
Copy Markdown
Collaborator

Raghuvar, I checked every ask you pushed in 6323e34, and they all work. Thank you. A deeper review then found more problems, so I fixed them myself and pushed 137c773 on top of your commit. You don't need to do another round. Please pull before you push again.

What changed:

  1. A line that doesn't parse now makes the converter exit 1. So does an entry with no usable recorded_at. Before, someone could replace a line with garbage and hide a deletion.
  2. The converter now reads every error code the host can write, like -1e+21 and -Infinity.
  3. A late decision now gets the timeout status, and a guardian_fault span gets the guardian_error status.
  4. Both writers read the join ids from one table, and a bad mapping is refused at load.
  5. Ids that nothing has verified are now escaped and limited in length.

Every suite passes on the merged tree, except posture.test.ts, which also fails on integration. PR #404 fixes that one. I broke the code on purpose in 73 ways, and tests caught 72. The last one doesn't change behavior.

Two things are still open, and neither is yours. Saba has item 6 of issue #284, so this PR stays Part of #284. @bar-capsule, duplicates exit 1 the same way gaps do, so harmless parallel hooks look like a lost line. Do you want a separate exit status?

I pushed last, so my approval doesn't count here. @bar-capsule @afogel, one of you needs to approve this one.

@rocklambros
rocklambros dismissed their stale review November 1, 2026 12:20

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.

@bar-capsule

Copy link
Copy Markdown
Collaborator

Reviewed the whole PR, including 137c773. The names come from otel-mapping.json with no fallbacks, the eight failure classes match the mapping and a test reads the JSON, and the load-time checks stop a bad mapping before any output is written.

One change, answering Rock's question: duplicates and recorded_at going backwards should print the line and exit 0. Both happen in a healthy run when hooks run at the same time, as Rock measured with 8 parallel hooks, so exit 1 on them would fire on normal sessions and teach operators to ignore it. Keep exit 1 for gaps, lines with no usable seq, lines that don't parse, entries with no usable recorded_at, and missing spans. That means:

  • reportConversion exits 1 only for those
  • the CLI tests for [1, 2, 2, 3], [1, 1, 3, 1, 1, 4, 6, 6] and the backwards case expect 0, with the same stderr line
  • the README bullet and the file header say which counts exit 1

#284 item 5 should say the same, and I'll note it there.

Also, please add 137c773's changes to the description: unparseable lines and missing recorded_at exit 1, unverified ids are escaped and bounded, and out-of-range timestamps are timed at now.

…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>
@drk-ram

drk-ram commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

@bar-capsule @rocklambros Pushed 4bfd577 on top of 137c773 and updated the PR description:

  1. Updated reportConversion in packages/guardian/src/audit-to-otlp.ts so duplicate seq values and recorded_at going backwards print the audit log order: ... line on stderr and exit 0, while seq gaps, lines with no usable seq (including unparseable lines), entries with no usable recorded_at, and missing spans exit 1.
  2. Updated the CLI tests for [1, 2, 2, 3], [1, 1, 3, 1, 1, 4, 6, 6], and the backwards-timestamp case in test/audit-to-otlp.test.ts to assert exitCode === 0 with the same stderr line.
  3. Updated the header comment in packages/guardian/src/audit-to-otlp.ts and the README bullet in packages/guardian/README.md, and added 137c773's changes to the PR description.

This branch has not been deployed

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

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants