Skip to content

feat(audit)!: consolidate audit log JSON entries into command_evaluations#333

Merged
fohte merged 8 commits into
mainfrom
fohte/audit-log-parsed-field
May 4, 2026
Merged

feat(audit)!: consolidate audit log JSON entries into command_evaluations#333
fohte merged 8 commits into
mainfrom
fohte/audit-log-parsed-field

Conversation

@fohte
Copy link
Copy Markdown
Owner

@fohte fohte commented May 4, 2026

Purpose

  • Extract the actual binary from audit logs with a one-line jq. Filtering on the raw command string mismatches on inline env prefixes (FOO=x helmfile ...) and on binary names buried inside argument bodies such as --prompt
  • The existing audit JSON places fields differently for single and compound (a && b etc.) commands, so consumers must branch on whether to read top-level matched_rules or sub_evaluations[].matched_rules

Approach

  • Drop AuditEntry.matched_rules and sub_evaluations; collapse them into a single command_evaluations: [CommandEvaluation]
  • Put command, action, matched_rules, eval_type, env, argv, redirects, pipe flat on CommandEvaluation so the shell-level parse data sits at the same level as the rule evaluation result
  • Non-compound input produces one element (eval_type: "primary"); compound input produces one element per branch (eval_type: "compound"); input with no runnable command (comment-only, parse failure) produces an empty array
{
    "command": "FOO=x echo hi && BAR=y cat /tmp/f",
    "command_evaluations": [
        {
            "command": "echo hi",
            "action": { "type": "allow" },
            "eval_type": "compound",
            "env": [{ "name": "FOO", "value": "x" }],
            "argv": ["echo", "hi"]
        },
        {
            "command": "cat /tmp/f",
            "action": { "type": "allow" },
            "eval_type": "compound",
            "env": [{ "name": "BAR", "value": "y" }],
            "argv": ["cat", "/tmp/f"]
        }
    ]
}
Design decisions

Whether single and compound entries share one shape

Decision Design Pros Cons
Chosen Both single and compound entries use the same command_evaluations: [] shape A single canonical jq path (.command_evaluations[]) covers both cases, eliminating the single/compound branch Breaking change for consumers that referenced the old matched_rules / sub_evaluations fields directly
Rejected Keep top-level parsed for single inputs, scatter per-branch parse data under sub_evaluations[].parsed for compound Pure additive change, no schema breakage Consumers still need an OR between top-level and sub_evaluations paths to handle both cases

Whether runok names binary / subcommand

Decision Design Pros Cons
Chosen Stay at the shell-parse layer; consumers read argv[0] themselves CLI-agnostic; runok carries no per-CLI knowledge Consumers must know to read argv[0]
Rejected Emit dedicated binary / subcommand fields jq becomes parsed.subcommand Subcommand position varies per CLI (git -C dir status puts it at argv[3]); runok would need a per-CLI table

Breaking changes

  • The top-level matched_rules and sub_evaluations keys on AuditEntry JSON are removed; their contents move into each command_evaluations[] element

intent(audit): let audit consumers filter by the real binary
  (e.g. `helmfile`) without re-implementing shell tokenisation in
  jq, which currently mishandles inline env prefixes like
  `FOO=x helmfile ...` and matches command names in `--prompt`-style
  argument bodies.
decision(audit): expose runok's existing `ExtractedCommand` shape
  (env / argv / redirects / pipe) verbatim. The data is already
  computed during evaluation; serialising it is the cheap path.
decision(audit): top-level `parsed` for single commands, per-branch
  `sub_evaluations[].parsed` for compound. Single inputs stay
  one-hop reachable from `.parsed.argv[0]`; compound inputs already
  forced consumers to walk `sub_evaluations`, so duplicating the
  data at the top would only add noise.
rejected(audit): unifying single + compound by always populating
  `sub_evaluations` with one entry — simpler jq but breaks every
  existing consumer that branches on `sub_evaluations == null`.
rejected(audit): emitting `binary` / `subcommand` as separate
  fields — subcommand position varies per CLI (`git -C dir status`
  has it at argv[3]) and runok would have to ship a per-CLI
  shaping table that drifts from upstream.
constraint(audit): backwards compat is preserved via
  `skip_serializing_if` on every new optional field; old jq
  filters that don't reference `.parsed` see no schema change.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a structured parsed field to audit log entries, providing shell-level tokenization results—including environment variables, arguments, redirects, and pipeline status—directly in the JSON output. This allows audit consumers to filter by binary or arguments without re-implementing shell parsing. The implementation updates the command extraction logic using tree-sitter, adds new serializable models, and includes comprehensive integration tests and documentation. Feedback was provided to refactor the new roundtrip tests into an existing parameterized test suite to adhere to the repository's style guide regarding test parameterization and naming.

Comment thread src/audit/model.rs Outdated
fohte added 3 commits May 4, 2026 14:12
intent(audit): collapse the single-vs-compound branching consumers
  had to write (top-level `parsed` / `matched_rules` for non-compound
  inputs vs. `sub_evaluations[].parsed` for compound) so a one-line
  jq filter on the actual binary works for both shapes.
decision(audit): drop top-level `matched_rules`, `sub_evaluations`,
  and the just-added optional `parsed`. Surface a flat
  `command_evaluations: [CommandEvaluation]` instead — one entry
  for non-compound (`eval_type: "primary"`), one entry per branch
  for compound (`eval_type: "compound"`), and an empty array when
  the input has no runnable command (comment-only / parse failure).
decision(audit): inline `env` / `argv` / `redirects` / `pipe`
  directly onto `CommandEvaluation` rather than nesting them under
  a `parsed` sub-object — consumers were already going to read
  `command_evaluations[].argv[0]`, so the extra hop bought
  nothing.
rejected(audit): keep both shapes side by side under a feature
  flag. Both shapes describe the same data with different
  conventions; carrying them long-term doubles the surface for
  consumers and the tests that pin the wire format.
constraint(audit): runok 0.x — breaking the audit JSON shape is
  permitted, but `releases/next.md` carries the migration so jq
  consumers know to pivot through `.command_evaluations[]`.
intent(audit): catch typos in `"primary"` / `"compound"` literals at
  compile time. The audit log JSON shape stays identical thanks to
  `#[serde(rename_all = "lowercase")]`, so consumers see no change.
decision(audit): mirror the precedent set by `SerializableAction`,
  which also exposes a Rust enum and serialises as lowercase strings.
…d-field

# Conflicts:
#	src/rules/command_parser.rs
@codecov
Copy link
Copy Markdown

codecov Bot commented May 4, 2026

Codecov Report

❌ Patch coverage is 92.10526% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.77%. Comparing base (49f6212) to head (8fa80b8).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/audit/model.rs 92.23% 8 Missing ⚠️
src/adapter/mod.rs 94.33% 3 Missing ⚠️
src/rules/command_parser.rs 93.10% 2 Missing ⚠️
src/rules/rule_engine.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #333      +/-   ##
==========================================
+ Coverage   88.74%   88.77%   +0.03%     
==========================================
  Files          53       53              
  Lines       12163    12314     +151     
==========================================
+ Hits        10794    10932     +138     
- Misses       1369     1382      +13     
Flag Coverage Δ
Linux 88.56% <92.10%> (+0.01%) ⬆️
macOS 89.92% <92.10%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fohte fohte changed the title feat(audit): add a shell-level parse result field to audit log JSON entries feat(audit)!: consolidate audit log JSON entries into command_evaluations May 4, 2026
fohte added 3 commits May 4, 2026 14:26
intent(releases): match the PR-link convention the rest of the
  Highlights / Bug Fixes entries already follow, so readers can
  jump from the changelog to the PR for the full discussion.
…d-field

# Conflicts:
#	docs/src/content/docs/releases/next.md
intent(releases): keep the release notes focused on the schema
  change runok actually owns. The `jq` Before/After examples
  drifted into telling consumers how to query their own audit
  pipeline, which the linked `cli/audit` reference already covers.
…ation

intent(audit): pre-#333 logs (top-level \`matched_rules\` plus
  \`sub_evaluations\`) were being skipped by \`runok audit\` with
  \`missing field 'command_evaluations'\`, hiding everything users
  recorded before they upgraded.
decision(audit): teach \`AuditEntry\` to deserialise via a shadow
  \`RawAuditEntry\` that reads both shapes, projecting the legacy
  one into \`command_evaluations\` (compound -> per-branch
  \`Compound\` records, non-compound -> a synthesised \`Primary\`
  record) before construction.
constraint(audit): only the read path changes; serialisation still
  emits the current schema, so old logs surface in the new shape
  without runok itself ever writing the legacy form again.
@fohte fohte merged commit dbea3e7 into main May 4, 2026
10 checks passed
@fohte fohte deleted the fohte/audit-log-parsed-field branch May 4, 2026 06:22
@fohte-bot fohte-bot Bot mentioned this pull request May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant