docs(adr): ADR-004 revision 1 decision cycle - redaction contract scope and ownership - #550
Merged
Conversation
…wnership) A decision document, not an implementation. It decides what the redaction contract promises and who owns each part of it, and decides nothing about how. Six rulings, each independently acceptable, amendable, or rejectable: 1. Where the contract binds: the crate/library API, generalizing the two lanes that already bind by construction. IPC/emit deferred; frontend egress explicitly out of scope as a boundary. 2. Equality scope: one analysis, caller-supplied identity. The security requirement is stated as a property, not an algorithm. 3. Ownership: grammar shared, projection per-lane, derivation shared, with a test for the distinction rather than a list. 4. Restricted vs Sensitive: give Restricted a real distinct behaviour rather than removing it or leaving it decorative. 5. Structural obligations: exhaustive construction enforced recursively by the compiler; self-description by type where the lane can express it. 6. Cross-lane correlation: out of scope, stated plainly, with a prohibition on presenting a shared derivation as delivering it. Token mechanics and migration are deliberately deferred. No source file changes. Records a direct contradiction inside the existing architecture: ADR-004 forbids stable cross-artifact correlation tokens and five of six Windows Intune lanes mint exactly those, with global stability stated as the design goal in their own module docs. Every code reference was opened and verified against origin/main at 2678f1f. Five inventory claims were found to be wrong or imprecise and are corrected in the document rather than silently applied, including that SCCM already projects by construction on its export path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds accepted ADR-004 Revision 1. It defines eight redaction and correlation rulings, records current-state findings and deferred decisions, and updates ADR precedence and status guidance. ChangesRedaction ADR governance
Estimated code review effort: 2 (Simple) | ~15 minutes Mergeability Score: 🟡 Moderate · up to This documentation-only change establishes redaction behavior for future implementations, but the current ADR still requires an impossible guarantee that every analysis produce a different token for the same input and retains a stale provisional equality statement. Those inconsistencies could lead to an unimplementable or misleading contract, so the PR should be corrected before merge. Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture/decisions/ADR-004-redaction-scope-revision-1.md`:
- Around line 474-479: Resolve the Restricted wire contract in ADR-004 before
accepting Ruling 4B: either require restricted fields to be omitted from
serialization, or update the invariant to explicitly allow a fixed marker while
forbidding raw values and derived equality. Align the related text at the other
Restricted contract section and add a serialization test covering the selected
representation.
- Line 741: Reset the ordered-list numbering in the section containing “The
shared-grammar claim” so the new list starts at 1: change its entries from 6.
and 7. to 1. and 2., including the corresponding item near the later referenced
location.
- Around line 370-372: Clarify the shared derivation’s domain definition in the
ADR by explicitly stating whether lane identity and field kind are included, and
define the comparison rule for tokens across domains. Reconcile the
corresponding statements at the referenced ruling sections so they consistently
specify whether cross-domain token equality is contractually impossible or
merely non-evidence of correlation.
- Around line 531-535: The ADR wording incorrectly attributes recursive
projection enforcement to rustc. Revise the discussed mechanism to state that
rustc only enforces exhaustive struct literals, then specify a separate lint,
type boundary, or test that rejects clone-and-mutate, struct-update syntax, and
Default-based construction while preserving the raw-data redaction guarantee.
- Around line 39-43: Update the reference guarantee in ADR-004 to state that
line-number references are provided “where applicable,” while preserving the
existing repository-root-relative wording. Ensure the guarantee accurately
covers directory and file references without line numbers, including the
referenced inventory entries.
In `@docs/architecture/decisions/ADR-004-redaction-scope.md`:
- Around line 3-8: Update the revision notice near
ADR-004-redaction-scope-revision-1.md to describe it as proposing rulings on
equality scope and ownership rather than deciding them. Preserve the statement
that the current document remains authoritative until the proposal is ruled on.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9614ceb9-53ba-493d-a0f8-b91458b5a050
📒 Files selected for processing (3)
docs/architecture/decisions/ADR-004-redaction-scope-revision-1.mddocs/architecture/decisions/ADR-004-redaction-scope.mddocs/architecture/decisions/README.md
Revision 1 was written as a proposal with six open questions and a recommendation attached to each. The owner has ruled, so the document now states decisions rather than options. The six questions become eight recorded rulings, each carrying the reasoning that supports it, one line on what a lane author must now do differently, and the mechanism that catches a violation (compiler, test, or review). Two rulings refine what the draft recommended and say so explicitly: - Ruling 3 replaces the draft's caller-supplied "analysis identity" with an opaque caller-owned RedactionContext. The crate does not interpret the value, derive meaning from it, or require it to name anything. This dissolves the draft's open question about which identity a caller should pass, and makes every testable property relational rather than semantic. - Ruling 4 forbids the fallback the draft left unruled. Configuration degrades its scope to generatedAtUtc alone when no scope is supplied. A lane with no context must now decline or emit with no equality; it may not substitute material that travels with the export. Ruling 6 sharpens Restricted from "a masked value" to no value-derived representation at all, enforced by one shared test asserting that two differing restricted inputs produce identical output. Ruling 7 states exhaustive construction recursively and splits the enforcement honestly: the compiler catches a new field only once the constructor is exhaustive, so the construction style itself is a review obligation. Re-verified against origin/main at f174012, which moved three of the draft's claims. Compliance no longer carries a private masking grammar (#546 deleted the fork; the lane re-exports the shared owner and pins byte-for-byte parity), its structural citations moved, and its defect is the inverse of the shape the draft described: non-exhaustive at the top level, exhaustive in a nested view. The grammar-owner row in shared-vs-workload-invariants.md also moved (#548). Compliance's private unsalted minter did survive, which is a useful demonstration that sharing the grammar and sharing the derivation are independent questions. Scope is unchanged and contract-only: no primitive named, no migration plan, no source file touched. The security requirement stays a property. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture/decisions/ADR-004-redaction-scope-revision-1.md`:
- Around line 850-857: Update the migration bullet in the section “What would
have to be true to decide the deferred items later” so it no longer claims that
nothing further is needed. State that Rulings 1 through 8 constrain the future
migration design, while the token algorithm, derivation, secret source,
encoding, existing-export behavior, compatibility window, and convergence order
remain unresolved.
- Around line 354-359: Revise the Decision statement so token equality expresses
equality preservation within a single analysis and scoped non-correlation across
analyses, without asserting an exact biconditional or impossible collision-free
behavior. Keep the existing keyed, domain-separated, collision-resistant
derivation requirement and its collision definition consistent.
- Around line 864-873: Revise the cross-export equality conclusion in the
discussion of Rulings 2 and 3: do not state an unconditional prohibition.
Document that exports sharing the same caller-supplied context may have equality
within one analysis, while exports from separate analyses are not required to
compare equal.
- Around line 327-332: Update the frontend-surface discussion around the
“negative” rule to avoid asserting that received values are already projected
today. Qualify that guarantee as applying only after the crate-boundary
implementation is complete, while preserving the existing decision to keep
frontend surfaces out of scope; do not bring IPC or emit boundaries into scope
unless required.
- Around line 614-619: Update the “What it obliges” section to explicitly
preserve the exception for lanes with genuinely different vocabularies to own
separate grammar rules, while requiring all other lanes to converge private
grammar on the owner. State the enforcement conditions clearly, including when a
lane author may define grammar and that projections remain the only permitted
contribution otherwise.
- Around line 3-8: The ADR notices overstate which redaction decisions are
finalized. In
docs/architecture/decisions/ADR-004-redaction-scope-revision-1.md:3-8, revise
the status notice to retain keying, derivation, algorithm, encoding, secret
source, and cross-artifact/session/export behavior as provisional while
preserving the accepted caller-owned context and single-analysis equality
decisions; in docs/architecture/decisions/ADR-004-redaction-scope.md:3-10,
update the status and notice so equality scope is not simultaneously presented
as both resolved and provisional.
- Around line 800-812: The invariant-status table must not claim that every lane
has a scope. Update the “Same-scope redaction preserves intended equality” and
“Different scopes do not accidentally create equality” entries to distinguish
context-present analyses from no-context constant-marker or decline paths
permitted by Ruling 4, and state that the no-context path has no scope.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 70548658-ab51-4b21-9e9e-89f5039dbed1
📒 Files selected for processing (3)
docs/architecture/decisions/ADR-004-redaction-scope-revision-1.mddocs/architecture/decisions/ADR-004-redaction-scope.mddocs/architecture/decisions/README.md
This was referenced Aug 11, 2026
adamgell
added a commit
that referenced
this pull request
Aug 12, 2026
* fix(intune): stop writing an app inventory to TEMP on every analysis run Every Intune analysis wrote %TEMP%/cmtrace-guid-diag.log, unconditionally. No operator asked for it, nothing cleaned it up, and it was never redacted. What it carried is an organisation's app inventory: every GUID registry entry with its application name, every event name enriched or missed with its GUID, and every download name with its content id and size. That is the same class of identifier the Intune lanes mask everywhere else, written in cleartext to a directory other users on the machine can read. This is developer instrumentation for diagnosing GUID enrichment. The summary an operator needs already goes to the application log; only the verbose per-entry trace went to the file. It is now collected only when CMTRACE_INTUNE_GUID_DIAG is set to a non-empty value, and nothing reaches disk otherwise. Implemented as a sink that implements fmt::Write and discards when off, so the eleven writeln! call sites are unchanged and stay interleaved with the enrichment logic they describe. An enabled-but-empty run writes no file either, rather than leaving an empty one in TEMP. Mutation-checked: making the sink always collect fails two of the four tests. This is the one item in #549's "related, same root cause" list that needs no boundary ruling from ADR-004. It is not an export that should have been projected; it is a write nobody requested. The export-boundary items in #549 and #556 still wait on #550. Refs #549. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(intune): write the opt-in trace where it cannot clobber or be clobbered Review findings on the gate itself. The trace went to a fixed name in the system temp directory opened with File::create, which truncates whatever is already there. That directory is world-writable, so a guessable name is the classic clobber and symlink-follow target, and this trace is opt-in precisely because its contents are sensitive. The name now carries the process id and a nanosecond stamp, and the file is opened with create_new, so two analyses cannot overwrite each other and the call fails rather than writing through something an unprivileged user planted. A failure is logged rather than silently swallowed. The test helper restored the environment variable after the body ran, which is skipped while unwinding. One failing assertion would have left the trace enabled for every test after it, turning a single real failure into a cascade of unrelated ones. It is an RAII guard now, so Drop restores during unwinding too. Also renamed a test that claimed more than it checked. It asserts the sink holds nothing, which is true, but its name implied an enabled run writes no file, and in the real analysis the pipeline summary always writes once the trace is on. A test passing for a reason its name denies is worse than no test. Mutation-checked: putting the fixed name and File::create back fails one test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(intune): make the opt-in trace owner-readable, and test the helper not the stdlib Three more review findings, and the first is the one that mattered. The trace file was created with the default umask, which leaves it world-readable, in a shared temp directory. That is the app inventory this whole change exists to stop leaking, sitting readable by every local user on the one path that does write it. It is opened 0o600 on Unix now. Windows inherits the per-user ACL of the profile temp directory. The write result was discarded, so a failed or partial write still logged guid_diag_written. Success is reported only once the bytes are down, which is the same "looks fine, is not" shape the rest of this change is about. The exclusive-open test asserted on fs::OpenOptions directly, so it proved the standard library works and would still have passed if the helper stopped using create_new. Path generation is split from opening so the helper itself can be handed a path that already exists, which is the case that matters. Mutation-checked separately: dropping the mode fails one test, dropping create_new fails another. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(intune): give the trace path a sequence, and a test that proves it matters Two analyses in one process can land inside a single clock tick, and a clock can move backwards. Either repeats the timestamp, and because the file is opened exclusively a repeat does not overwrite anything but does lose the trace the operator asked for. A process-local atomic sequence makes the path distinct by construction rather than by luck. The first version of the test called trace_path in a loop and passed with the sequence removed, because successive clock reads on this machine differ anyway. It proved nothing about the thing it was named for. Naming is now split from the clock so the stamp can be held fixed, which is the only way to exercise the collision case; with the sequence removed it fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Revision 1 resolves the equality scope and the caller-owned context, not the key, algorithm, encoding, secret source, or cross-export behaviour. The status notices, the frontend projection claim, the token-equality biconditional, the grammar-ownership exception, the invariant table, the migration bullet, and the cross-export answer are all restated to match what the rulings actually decide, and the original ADR's notice and status no longer describe equality scope as both resolved and provisional.
adamgell
marked this pull request as ready for review
August 13, 2026 00:49
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This decides the contract only
Token mechanics and migration are deliberately deferred. This PR names no
cryptographic primitive, no key length, no encoding, and no library. Ruling 2
states the security requirement as a property, and implementation research
picks the algorithm after the contract is accepted. Baking an algorithm in now
would let an implementation detail masquerade as an architectural commitment,
which is how the current unsalted-FNV-1a monoculture arrived in the first place:
four lanes copied a hash function, and the hash function became the contract.
This PR changes no source file. Three files under
docs/architecture/decisions/only.Why now
ADR-004 says at
ADR-004-redaction-scope.md:5that new reducers "must notintroduce stable identifier tokens intended for cross-artifact, cross-session,
or cross-export correlation."
Five of the six Windows Intune lanes mint exactly those, deliberately, with
global stability stated as the design goal in their own module docs
(
crates/cmtraceopen-parser/src/intune/apps/windows/common/redaction.rs:9-12;crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/redaction.rs:33-35).That is a direct contradiction inside the accepted architecture, and adapter
authors have been resolving it by copying whichever neighbouring lane they read
first. It has to be resolved before the next lane is written.
The six questions requiring your ruling
Each is independently acceptable, amendable, or rejectable. Each has options and
a recommendation.
emitdeferred pending open question 1. Frontend file-save and clipboard explicitly out of scope as boundaries, because per-egress hygiene is the arrangement that produced #549.RestrictedvsSensitiveRestrictedemits no derived value at all, so two differing inputs produce identical output. Removing the level is the second-best answer; leaving it decorative is not an answer.clone()can carry over.What it deliberately does not decide
emitboundary binds (deferred pending open question 1).Open questions recorded rather than invented
constructs any of their snapshot types today.
analysis_scope? A case number, acollection run id, and a session id have different blast radii, and the
choice determines what "one analysis" means.
so, Ruling 2 should be amended before acceptance, not after.
Verification
Every code reference is repository-root relative with a line number, was opened
and read while writing, and resolves in range (78 distinct citations, checked
mechanically as well as by hand). Verified against
origin/mainat2678f1fb.Five inventory claims were found to be wrong or imprecise and are corrected
in the document rather than silently applied, because the corrections change
what some rulings must cover. The most consequential:
two; the second is
crates/cmtraceopen-parser/src/sccm/evidence.rs:329-343,reachable only through
crates/cmtraceopen-parser/src/sccm/ingest.rs:9. It isarguably the better of the two, and it strengthens Ruling 1.
clone()+mutate" undercounts. There arethirteen projections and eight construct non-exhaustively.
Two findings the inventory did not report are also recorded: three of the six
Windows lanes carry a private redaction grammar rather than a private
projection, which is outside what divergence 7 licenses; and the frontend
already implements the
Restrictedbehaviour the Rust export lanes do not(
src/workspaces/esp-diagnostics/esp-view-model.ts:119-122), which is thestrongest argument for Ruling 4.
Closes nothing. Issue #549 is cited as evidence of a defect class, not fixed
here.
Suggested next step:
/code-reviewon this branch before ruling. The reviewersubagent is materially weaker than the slash command on foundational work like
this, and the rulings here will constrain every lane written after them.
🤖 Generated with Claude Code
Summary by CodeRabbit