Skip to content

fixture: redstamp approve-path with host-observation sidecar - #98

Open
rajudandigam wants to merge 4 commits into
askalf:mainfrom
rajudandigam:fixture/redstamp-approve-path
Open

rajudandigam wants to merge 4 commits into
askalf:mainfrom
rajudandigam:fixture/redstamp-approve-path

Conversation

@rajudandigam

Copy link
Copy Markdown

Summary

  • Add synthetic corpus/redstamp/approve-path with a real sha256 hash chain, checkpoint tip, and host-observation sidecar for decision=approve / tier=red.
  • Extend the redstamp adapter to join host observations and refine approve outcomes:
    • declined + complete window + not invoked → plumbline denied
    • missing / incomplete / join mismatch → ok with host_observation.status: unknown (fail closed)
  • Verify optional audit checkpoints so a truncated tail cannot pass chain verify alone.
  • Keep why[] tainted (mine hosts only; never copy verbatim).

Why

redstamp’s audit log records the human-approval decision, not whether the host invoked the tool. Treating approve as allow hides the non-invocation case. Chain integrity does not prove observer truth.

Test plan

  • npm test — 225 pass / 0 fail
  • Maintainer review of host_observation / audit_hash pre-normalize fields

Fill the approve-decision corpus gap: join an independent host observation
to refine approve outcomes (declined+complete → denied; incomplete/mismatch
fail closed), and verify optional audit checkpoints for truncated tails.
@github-actions github-actions Bot added adapters Log adapters and the envelope corpus Corpus and evaluation harness tests Test suite and CI size/L 200-799 hand-written lines labels Oct 10, 2026

@sprayberry-redline sprayberry-redline 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.

Request changes: conflicting observations can produce an authoritative non-invocation result. rule:none

P1: Reject conflicting observations for the same join key (src/adapters/redstamp.mjs:373-377)

observation = candidates.find((o) => {
  const opOk = o.operationId === undefined || o.operationId === rec.operationId;
  const attOk = o.attemptId === undefined || o.attemptId === rec.attemptId;
  return opOk && attOk;
}) ?? null;

The index retains multiple observations for a hash, but this selects the first matching one without checking the others. For an approve record, supply two observations with the same hash, operationId and attemptId: first {humanResponse: 'declined', targetToolInvoked: false, windowComplete: true}, then {humanResponse: 'accepted', targetToolInvoked: true, windowComplete: true}. The first record reaches the declined branch and produces outcome: 'denied' and status: 'not_invoked', despite contradictory invocation evidence. Reversing the sidecar order changes the result to ok / invoked. An ambiguous sidecar must not authorize a non-invocation claim merely through its ordering.

Collect all matching observations before deriving the outcome. If their invocation or response evidence conflicts, return the existing unknown fallback rather than selecting the first. Pin the conflicting pair in both input orders.

The checkpoint comparison usefully covers tail truncation that chain verification alone cannot detect. The complete five-file diff and adapter normalization context were inspected. No test-suite result is published at this head; the only completed check reported is triage, so the stated 225 passing tests are not independently confirmed by CI.

Collect all join-key matches before deriving approve outcomes. Conflicting
invocation/response/window evidence stays unknown in both sidecar orders;
consistent duplicates and checkpoint controls remain.

@sprayberry-redline sprayberry-redline 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.

Request changes: conflicting executor results still make the outcome depend on sidecar order. rule:none

P2: Include executor results in duplicate conflict detection (src/adapters/redstamp.mjs:406-416)

if (responses.size > 1 || invoked.size > 1 || windows.size > 1) {

The fallback still selects:

observation = matches[0];

For one approve record, supply two observations with the same hash, operationId and attemptId, both with humanResponse: 'accepted', targetToolInvoked: true and windowComplete: true, but with executorResult: 'ok' and executorResult: 'error'. All three sets have size one, so this treats the observations as consistent. outcomeFor then reads the first observation's executor result: the first ordering produces outcome: 'ok', and reversing it produces outcome: 'error'. Both also publish the selected executor result as authoritative host evidence. The conflict check needs to cover the field that determines that outcome, not just response, invocation and window completeness.

Compare executorResult (with missing values normalized to 'unknown') alongside the other evidence fields before selecting a duplicate. Route contradictory executor results through the conflict fallback and cover the accepted/invoked pair in both orders.

The revised join resolves the declined-versus-invoked conflict from the previous review, and the checkpoint test covers truncated tails. Review covered the five-file diff and the adapter's outcome derivation. CI currently reports only a passing triage job, with no published test-suite result; the reported local test count is not confirmed by CI.

Include executorResult in duplicate conflict detection so accepted/invoked
pairs with ok vs error cannot authorize an outcome by sidecar order.

@sprayberry-redline sprayberry-redline 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.

Request changes: use the surrounding module's plain wording in the added comments, messages and test name. rule:reads-as-generated

P3: Remove the added parenthetical commentary and long-dash phrasing (src/adapters/redstamp.mjs:425, test/redstamp-adapter.test.mjs:165, corpus/redstamp/approve-path.jsonl:2).

// Consistent duplicates — first is fine (order-independent evidence).
test('approve without host observation stays ok — invocation unknown', () => {
# Synthetic only — never execute these commands.

The duplicate-selection comment explains why the patch is correct rather than what the branch does; delete it. Use a short test name and ordinary sentence punctuation, consistent with the surrounding tests. Apply the same punctuation cleanup to the other added comments and reason strings in the adapter and the test-plan line in the description.

test('approve without host observation has unknown invocation', () => {

The executor-result conflict is fixed: contradictory accepted/invoked observations now take the unknown-evidence fallback in either order, and the new regression checks both orders without publishing a selected executor result. Review covered the five-file diff and the adapter's outcome derivation. CI currently publishes a passing triage job but no test-suite result; local tests were not rerun.

@askalf

askalf commented Oct 12, 2026

Copy link
Copy Markdown
Owner

Thanks for following through on #143. This is the fixture we asked for there, and both rounds of fixes for conflicting observations were quick and correct.

Your core point holds, and it is a real gap in plumbline today: a redstamp approve record does not say whether the call ran. What approve means depends on the host:

  • redstamp mcp-proxy blocks the call unless --allow-approve is set.
  • redstamp-hook in strict mode asks the user, and the answer never reaches redstamp.
  • redstamp-hook otherwise defers to Claude Code's own permission rules.

plumbline maps all three to ok, so a call the proxy refused is reported as a success.

I'd like to land this in two pieces.

In this PR

  1. Keep the approve-path fixture and the checkpoint check. The check catches a truncated tail that the chain alone cannot. Three changes so it agrees with redstamp's own verifyAuditFile:
    • Pin the prefix, not the length: the count-th chained record must have hash head, and a log that has grown past the checkpoint still verifies. As written, a live log checked against an older checkpoint emits a false audit.checkpoint_mismatch.
    • Count chained records only. redstamp-hook writes unchained records when its daemon is down, and those should not count toward count.
    • Make it reachable from the CLI. scanRedstampAudit doesn't pass a checkpoint through, so plumbline scan --adapter=redstamp never runs the check. Reading <log>.chk when it exists would cover it: that is the file redstamp's checkpoint option writes and verifyAuditFile reads by default.
  2. No new top-level event fields. audit_hash and host_observation aren't in schema/trajectory.schema.json, which sets additionalProperties: false and is the published contract per SPEC.md. The runtime normalizer is lenient, which is why the suite passes.
  3. Fixture records with only the fields redstamp writes, so no operationId or attemptId.

In redstamp

The invocation evidence should come from redstamp itself rather than from a sidecar format nothing writes yet. Each verdict should record what was enforced (blocked, asked, or forwarded), and the ask path needs a PostToolUse record so that whether the tool ran is observed, not inferred. Your join and fail-closed rules carry straight over to those records: "not invoked" only from a complete window, and conflicting evidence becomes unknown. That is askalf/redstamp#219, and it's yours if you want it.

If it's easier, I can make the trims on your branch. Either way I'll handle the punctuation note from the last review round.

@sprayberry-redline sprayberry-redline 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.

Verdict: request changes. The observation conflict handling now makes both response and executor-result conflicts order-independent. The requested cleanup of added explanatory prose remains incomplete.

1. Blocking: src/adapters/redstamp.mjs:425

      // Consistent duplicates — first is fine (order-independent evidence).

This added comment narrates the patch's correctness rather than documenting behavior, and its long-dash phrasing remains from the earlier requested cleanup.

Suggested fix:

Delete the comment; the surrounding duplicate comparison and assignment already state the behavior.

rule:reads-as-generated

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

Labels

adapters Log adapters and the envelope corpus Corpus and evaluation harness size/L 200-799 hand-written lines tests Test suite and CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants