feat(sync): opt-in git attribution spans on sync push --attribution - #848
feat(sync): opt-in git attribution spans on sync push --attribution#848Enclavet wants to merge 3 commits into
sync push --attribution#848Conversation
|
Intentionally made this opt-in. As the remote repo name and pr-links might be sensitive. |
iamtoruk
left a comment
There was a problem hiding this comment.
Thanks — the core of this is strong work, and the review went deep because this is the product's most privacy-sensitive surface. What held up very well under adversarial testing: the credential stripping survived 48 hostile remote forms (userinfo tokens, GitLab subgroups, scp-forms with colons in usernames, query-string tokens — zero leaks, no ReDoS), --dry-run is provably silent on the wire (zero /v1/traces POSTs against a live collector), flag-off behavior is byte-identical to main, the yield.ts extraction is behavior-preserving across 4,186 real sessions, and reusing the #660 plumbing (backoff, assertHttps, ledger) was exactly right.
Two egress bugs block the merge — both demonstrated end-to-end against a live mock collector:
1. The cwd fallback egresses unrelated (possibly confidential) repos. buildRepoGroups does identity = projectIdentity ?? cwdIdentity (src/yield.ts:390-392, 574). When a session's project path no longer resolves to a work tree (deleted/renamed dir, or sessions run outside a repo), attribution inherits whatever repo the user's shell is in at push time. Reproduced: pushing from inside a private repo emitted repo: "github.com/secret-org/nda-client-repo" plus its commit SHAs, attributed to a session that never touched that repo. In local yield this fallback is a harmless heuristic; on the sync path it's both a privacy leak and false attribution. Suggested fix: on the attribution path, drop the group (or at minimum repo + commits) whenever identity came from the cwd fallback rather than the project's own path.
2. Windows drive-letter paths defeat the "local repos are never sent" guarantee. The scp-like branch matches C: as a host (src/yield.ts:159-160): "C:/Users/alice/private/repo" → repo: "c/Users/alice/private/repo", and because repo is non-null, commits are sent — directly contradicting the doc line "Commits in repos with no network remote are never sent," with the user's local filesystem path as the emitted identity. Suggested fix: reject single-character hosts, or match /^[a-zA-Z]:[\\/]/ before the scp-like branch.
3. Tests for the above. The new test file is genuinely good (live collector, wire assertions), but it covers only 3 basic normalize cases — none of the adversarial forms, no Windows path, no --dry-run no-network assertion, no flag-off assertion. All the findings above would have been caught by that corpus.
Should-fix (non-blocking but please consider in the same pass):
git.pr_linksfrom host-emitted journal entries passes only a truthy-string check (src/parser.ts:1265upstream) — arbitrary strings of arbitrary length reach the endpoint. A shape check (URL, allowlisted hosts) + per-session cap would close it. Relatedly, PR links are sent even whenrepois null (deliberate and defensible, but the docs' "never sent" sentence reads more absolute than reality — worth reconciling).- Attribution items have no
MAX_PER_PUSH-style cap; a first--since all --attributionpush on a long history has no valve. - A CHANGELOG
## Unreleasedentry — a privacy-relevant opt-in flag belongs in release notes. - The PR body's attribute table under-declares vs the wire:
codeburn.device_id,codeburn.attribution_methodology, and commit timestamps (span start times) also ride along. Your docs/sync/README.md discloses the timestamps honestly — the PR body should match it.
Nice-to-have: case-insensitive .git strip and slash collapse for join-key stability (…/Repo.GIT and doubled slashes currently split one repo into two keys); document that only origin is read.
Happy to re-review promptly — the shape of this feature is right and the plumbing reuse makes it an easy yes once the egress edges are closed.
|
Thanks for the thorough review — the two privacy findings were real, and both are now fixed in ce800d8, along with all the should-fixes. Point by point: 1. cwd-fallback egress — fixed at the source. One consequence worth calling out: I went further than dropping repo+commits. Fallback sessions are excluded from the tightest-window competition entirely — otherwise a fallback session with a tighter window could still steal a commit from the genuine session that owns it (the genuine session would then read as abandoned, a false negative your suggested fix wouldn't have caught). That's also tested ( 2. Windows drive letters — fixed. 3. Tests. The corpus you asked for is in:
Should-fixes — all taken:
Full suite: tsc clean, 118 tests green across the yield/sync/attribution suites (25 new in this PR). |
|
@iamtoruk Can you take a look at this PR when you get a chance? |
iamtoruk
left a comment
There was a problem hiding this comment.
Re-reviewed after ce800d8. The two blockers from the last round are genuinely closed: the cwd-fallback egress gate holds (fallback sessions emit no repo and no commits, and can no longer steal a commit from a genuine session's window), and the Windows drive-letter path is rejected including the git@C:/foo backtrack you called out. The computeYield to buildRepoGroups extraction is behavior-preserving, the sendBatches to sendBatchesCore refactor has no drift, ledger correctness holds (nothing ledgered without server acceptance, 429 retry is idempotent), the scp-like regex is ReDoS-safe, and flag-off behavior is byte-identical to main. All verified by execution against real temp git repos and a mock collector.
But the same credential-egress class the last round flagged is still open on three independent paths, all reproduced end to end against this branch. The headline guarantee ("a token embedded in an https remote must never leave the machine") is falsifiable, so I have to keep this at changes-requested.
Blocking
1. normalizeRemoteUrl leaks credentials via regex backtracking (src/yield.ts:167)
A remote that carries userinfo but is not caught by the :// branch (missing a slash, or no scheme at all) falls into the scp-like branch, where the optional (?:[^@/]+@)? userinfo group backtracks and abandons itself. The host then matches the credential prefix and the rest of the string, token included, is dumped into the path:
x-access-token:ghp_LIVETOKEN_abcdefghijklmnop@github.com/acme/private-repo.git
-> git.repo = "x-access-token/ghp_LIVETOKEN_abcdefghijklmnop@github.com/acme/private-repo"
https:/user:ghp_TOKEN@github.com/org/repo.git (one dropped slash)
-> "https/user:ghp_TOKEN@github.com/org/repo"
oauth2:glpat-TOKEN@gitlab.com/org/repo.git
-> "oauth2/glpat-TOKEN@gitlab.com/org/repo"
Well-formed https:// remotes still strip correctly (fuzzed 5 schemes x 5 userinfo forms x 4 paths, zero leaks), so the trigger is a malformed or hand-edited origin, not the default. But the invariant is stated as absolute, and this makes it false. Excluding @ from the host class (the ce800d8 fix) does not help, because it only blocks @ inside the host, not the backtrack. Suggested fix: split on the first @ before host matching, then reject any host or path that still contains @.
2. Transport-helper remotes leak local secrets, and git.repo has no shape or length guard (src/yield.ts:167,188)
sanitizePrLinks strictly shape-checks PR URLs, but the repo identity gets no equivalent check, so git remote get-url origin output reaches the wire verbatim:
ext::ssh -i /Users/me/.ssh/id_ed25519_work git@github.com %S /acme/private.git
-> git.repo = "ext/:ssh -i /Users/me/.ssh/id_ed25519_work git@github.com %S /acme/private" (local SSH key path)
codecommit::us-east-1://MyAwsProfile@MyRepo
-> "codecommit/:us-east-1://MyAwsProfile@MyRepo" (AWS profile name)
git-remote-ext and git-remote-codecommit are shipped transports, so these are real origin values. A 10,000-char remote also produces a 10,000-char git.repo. Suggested fix: allowlist the repo identity to a sane host/path shape (the same discipline sanitizePrLinks already applies) and bound its length.
3. sanitizePrLinks passes userinfo, query strings, and fragments (src/yield.ts sanitize path)
It validates only url.pathname, so everything else in the URL rides along:
https://alice:ghp_TOKEN@github.com/acme/widget/pull/5 -> kept verbatim (credential)
https://github.com/acme/widget/pull/7?notification_referrer_id=xyz -> query kept
https://github.com/acme/widget/pull/6#fragment -> fragment kept
This is the function whose whole job is gating what leaves the machine, and it is the most reachable of the three (PR links are copy-pasted by humans and routinely carry referrer tokens). Suggested fix: reconstruct the URL from origin plus pathname only, dropping username, password, search, and hash.
Should-fix (not blocking)
- Stale session attribution is never retracted (double-count). When a commit migrates to a later-parsed session with a tighter window, the earlier session's
codeburn.session.attributionspan is not re-emitted, so a backend summinggit.commit_countacross session spans permanently double-counts. The documented(git.repo, git.sha)upsert saves the commit span but not the session span. Reachable whenever a second session for the same repo is parsed on a later push (still-being-written transcript, session that grew its window). - Session dedup key omits project and both timestamps (
src/sync/otlp.ts:188). An ongoing session's span end time and project name are frozen at first send and never corrected, which defeats the stated purpose of the end timestamp. - Attribution summary line can claim success on a failed push (
src/sync/cli.ts:385). On a rate-limited or server-error attribution push, nothing is printed and it still writesAttribution: N facts synced. The usage path messages both outcomes; please mirror it. - Attribution spans can emit
endTimeof 0 or less thanstartTimewith no clamp (src/sync/otlp.ts:236) when a timestamp is malformed or out of order. The usage builder guaranteesend = start + 1ms; the attribution builder should too. - Case-sensitive
.gitstrip splitsRepo.GITand doubled slashes into a second join key. Minor data-quality wart, still worth closing for stable joins.
Verification notes
CI is green and the new attribution suites pass. The 2 failing tests in the full local run (parser.test.ts copilot purge and durable-orphans) reproduce identically on base main e7576e7, so they are pre-existing and not from this PR.
The shape of the feature is right and the plumbing reuse is exactly what I wanted. Please close the three egress paths (they share one root: no single strict sanitizer gates the repo identity and PR links the way the remote :// branch does), and I will re-review promptly.
Expose the yield session-to-commit correlation through codeburn sync so backends can join AI usage to git activity without local git hooks. - yield: export normalizeRemoteUrl (host/org/repo; credentials, ports, and .git stripped) and computeAttributionRecords, which reuses the exact repo-grouping + tightest-window attribution from computeYield (extracted into a shared buildRepoGroups) and joins in the normalized origin remote and session prLinks. - otlp: two new span types sharing the session traceId — codeburn.session.attribution (git.repo, git.pr_links, git.commit_count) and codeburn.commit (git.sha, git.in_main, git.was_reverted). Resource attribute codeburn.attribution_methodology=timestamp-window marks the attribution as inferred. - push: generic send core reused by usage and attribution batches. Dedup keys encode mutable state (inMain/wasReverted), so a state transition re-sends the updated fact while identical states dedupe via the existing sent-ledger. - cli: opt-in --attribution flag on sync push (dry-run aware); commits in repos with no network remote are never sent. AI-Origin: human
…paths, PR-link validation Review findings on the --attribution PR: - Privacy: sessions whose project path no longer resolves inherited the cwd-fallback repo identity, egressing whatever (possibly confidential) repo the user pushes from and falsely attributing its commits. buildRepoGroups now tracks per-session identity provenance; the attribution path excludes fallback sessions from commit attribution entirely (no repo, no commits, PR links only) — they also can no longer steal a commit from a genuine session's window. - Privacy: Windows drive-letter paths (C:/..., C:\..., drive-relative) parsed as scp-like remotes, emitting local filesystem paths as repo identities. normalizeRemoteUrl rejects drive letters and single-character hosts (dotless intranet hosts still accepted). - Hardening: PR links are shape-checked before sending (https, /org/repo/pull/N path, <=256 chars, max 20 per session) — upstream parsers only truthiness-check them. - Safety valve: MAX_ATTRIBUTION_PER_PUSH (10k) caps a first --since all --attribution push; dry-run reports the cap. - Tests: adversarial normalize corpus, cwd-fallback egress repro, commit-stealing prevention, PR-link sanitization, and CLI-level tests (mock IdP + collector): dry-run sends nothing to the traces endpoint, flag-off emits no attribution span names on the wire. - Docs: reconciled the 'never sent' wording with reality (PR links ride even when repo is null; device_id/methodology/timestamps disclosed). CHANGELOG Unreleased entry added. AI-Origin: human
…CLI hardening Review rounds 2-3 + self-review on --attribution: Credential egress (round 2): - normalizeRemoteUrl: scp userinfo expressed as an optional regex group let backtracking re-parse a credential prefix as host:path (x-access-token:ghp_...@host/repo -> token in git.repo). Userinfo is now split off at the first @ BEFORE any host matching. - Positive validation (allow-list) as the final gate on EVERY branch: host must be hostname-shaped, every path segment repo-shaped, total identity <= 200 chars. Kills transport-helper remotes (ext:: leaks local SSH key paths, codecommit:: leaks AWS profile names), residual @, spaces/colons, and unbounded strings. - sanitizePrLinks: links are rebuilt from origin + pathname — userinfo, query strings, and fragments are dropped instead of passed through; collapsed duplicates dedupe. Attribution correctness (round 3 + self-review): - Double-count fix with precise retraction semantics: when a commit migrates to a later-parsed tighter-window session, the loser re-emits git.commit_count=0. Empty records are emitted ONLY on a true loss in THIS computation (lostCandidacy) — a commit that merely aged out of the --since range was lost to nobody, and retracting it would permanently zero a still-correct server-side count. The sync layer additionally requires a prior ledgered state for the session. - Session dedup key includes project + both window timestamps, so ongoing sessions re-emit with corrected span times. - Span end times clamped like the usage builder (never 0, never earlier than start + 1ms). - CLI mirrors the usage path on attribution push failures instead of claiming success. - Identity normalization: case-insensitive .git strip, doubled path slashes collapse. AI-Origin: human
28ac117 to
50c8251
Compare
|
All findings from both rounds are fixed in 50c8251. Point by point, then one additional bug I found while re-auditing that I want to flag explicitly. Housekeeping first: the branch was rebased onto current main and the fix rounds squashed, so earlier inline comments will show as outdated. Final shape is three commits: the feature (1bf7206), round-1 fixes (ccee28a), and rounds 2–3 (50c8251). Round 2 — credential egress1. Backtracking credential leak — fixed with your suggested approach. You were right that the 2. Transport-helper remotes / no shape guard — fixed with positive validation. A final allow-list gate now runs on every branch's output: host must be hostname-shaped, every path segment repo-shaped ( 3. sanitizePrLinks passthrough — fixed by reconstruction. Links are rebuilt from Beyond the unit corpus, I ran a fuzz verification: 225 token-bearing remote forms (5 token styles × 5 userinfo prefixes × 9 templates including malformed https and transport helpers) — zero tokens in any output — plus a full-pipeline run against real repos with planted tokens, scanning every wire byte: tokens and usernames absent, identities and sanitized PR links still transmitted. Round 3 — attribution correctness4. Stale session double-count — fixed with retraction spans. When a commit migrates to a later-parsed tighter-window session, the losing session now re-emits with One refinement you should scrutinize: while testing this I found the naive version of the fix ("retract whenever a previously-sent session computes empty") introduces a worse bug — a commit that merely ages out of a rolling 5. Session dedup key omissions — fixed. The key now hashes project + both window timestamps, so an ongoing session whose window grew re-emits with corrected span times. Accepted cost worth noting: an active session mints one new ledger entry per push while its window grows; the 6-month ledger prune bounds it. 6. Lying summary line — fixed. Rate-limited and server-error attribution outcomes now print the same style of message as the usage path, and the summary line reads 7. End-time clamp — fixed, matching the usage builder's guarantee: never 0, never earlier than start + 1ms. Tested with out-of-order and malformed timestamps. 8. Verificationtsc clean; 127 tests across the yield/sync/attribution suites (34 attribution-specific: unit corpus with every payload from both rounds as named cases, plus CLI-level tests against a mock IdP + collector). |
What
Adds an opt-in
--attributionflag tocodeburn sync pushthat sends the session→commit correlationcodeburn yieldalready computes locally, so a telemetry backend can join AI usage to git outcomes (merged / never merged / reverted) — without teams having to install per-developer git hooks.codeburn sync push --attribution codeburn sync push --attribution --dry-run # counts only, sends nothingWhy
codeburn synctoday answers how much AI was used (tokens, cost, model, project). It cannot answer where that AI output landed: which commits came out of a session, whether they shipped to main, or whether they were later reverted. Teams that want outcome metrics (AI-to-merge ratio, defect/revert rates on AI-assisted code) currently have to bolt on commit-trailer git hooks — a per-developer install with its own drift and maintenance burden.But codeburn already has the hard part.
codeburn yieldresolves each session's project to a canonical repo (monorepo subdirs and worktrees collapse viagit-common-dir), attributes commit SHAs to sessions by tightest timestamp window, and computesinMain/wasRevertedper commit. That analysis just never left the local CLI report. This PR exposes it through the existing sync channel.What gets sent (only with the flag)
Two new span types, sharing the session's traceId with the existing usage spans (
deriveTraceId(sessionId)), so receivers correlate cost and attribution with no extra key:codeburn.session.attribution— one per session with joinable evidence:ai.session_idabc123…ai.projectmy-appgit.repogithub.com/acme/widget(normalizedoriginremote)git.pr_links["https://github.com/acme/widget/pull/12"]git.commit_count2The span's start/end times are the session window itself.
codeburn.commit— one per commit attributed to a session:git.sha4f2a…git.in_maintruegit.was_revertedfalsegit.repo,ai.session_id,ai.projectA resource attribute
codeburn.attribution_methodology: timestamp-windowmarks the attribution as inferred (same self-declared heuristic ascodeburn yield), so backends can label it honestly versus declared sources like commit trailers.Design decisions
git remote get-url originis normalized tohost/org/repo: scp-like (git@host:org/repo.git),ssh://(user + port dropped), andhttps://(embedded credentials stripped — a token in an https remote must never leave the machine) all collapse to the same key. Local-only repos andfile://remotes have no server-side identity: their commits are never sent. A session in such a repo still emits a record when it carries PR links (the PR URL embeds the repo).inMain/wasReverted(and the session key hashes repo + PR links + commit states). Identical facts dedupe via the sent-ledger exactly like usage spans; a state transition (commit merges to main, or gets reverted) mints a new key and the updated fact is re-sent on the next push. Receivers should upsert by(git.repo, git.sha).$summary; a separateAttribution: N facts syncedline is printed.--attribution, behavior is byte-identical to today. With it, what leaves the machine is: normalized repo remote, commit SHAs + timestamps, PR URLs, and the merged/reverted booleans. Never code, diffs, paths, prompts, or bash commands. Documented indocs/sync/README.md.Implementation
src/yield.ts: the repo-grouping loop insidecomputeYieldis extracted intobuildRepoGroups(verbatim; grouping semantics unchanged) and shared with a newcomputeAttributionRecords(projects, range, cwd). Also exportsnormalizeRemoteUrl.computeAttributionRecordstakes already-parsed projects, sosync pushdoesn't re-parse.src/sync/otlp.ts:flattenAttributionRecords, dedup key builders,buildAttributionOtlpPayload,batchAttributionItems.src/sync/push.ts: the send loop is generalized intosendBatchesCore(the publicsendBatchessignature and behavior are unchanged); addscollectUnsentAttribution+sendAttributionBatches.src/sync/cli.ts: the--attributionflag, dry-run reporting, and summary output.Testing
tests/sync-attribution.test.ts): remote normalization (ssh/scp/https/credentials/local), record computation against real temp git repos (remote join, tightest-window ownership, no-remote handling, empty-session omission), dedup key state transitions, OTLP span shape, and the send+ledger pipeline against a mock OTLP server (success ledgering, 5xx not ledgered).yield*.test.ts,sync*.test.ts) —computeYieldbehavior and thesendBatchescontract are untouched.git reverted commit) pushed through the full pipeline to a live local HTTP server. Verified: correctin_main/was_revertedflags from real git, credential stripped from anhttps://user:token@…remote with a full-payload scan, idempotent re-collect after ledgering, and correct 2-fact re-send after merging the feature branch (state transition).Compatibility
sync push, payload shape, discovery doc, or auth.tscclean; no new dependencies.Caveats (by design)
codeburn yielddocuments. The methodology resource attribute exists so dashboards can label inferred vs. declared attribution.wasRevertedsignal only detects standardgit revertbodies ("This reverts commit <sha>"), matching yield's existing detection.Possible follow-ups (not in this PR)
sync.attribution: trueconfig option so scheduled pushes don't need the flag.originwhen both exist (origin may point at a fork).