feat(sync): push git attribution spans, with the hardening that followed - #932
Open
ozymandiashh wants to merge 2 commits into
Open
feat(sync): push git attribution spans, with the hardening that followed#932ozymandiashh wants to merge 2 commits into
ozymandiashh wants to merge 2 commits into
Conversation
ozymandiashh
marked this pull request as draft
August 5, 2026 10:03
Collaborator
Author
|
Converted to draft: this was opened with only a self-review by the model that wrote it, which is not the review bar this batch is held to. Independent review by two other reviewers is running now; I will mark it ready once both have passed and any findings are applied. Apologies for the noise. |
ozymandiashh
force-pushed
the
fix/port-sync
branch
from
August 5, 2026 10:13
3527dcf to
616dec3
Compare
Collaborator
Author
|
Merge-order blocker, from independent re-review. This PR adds a new attribution payload that sends So landing this alone ships new consumers of the unkeyed construction, and my earlier claim in the commit message that this is "not a D1 violation" is contested: one reviewer argued Concrete requirement: #931 must land before this, and whoever merges should reconcile the one-argument call sites this PR adds against #931's keyed signatures. I would rather flag this than have it discovered at merge. |
ozymandiashh
marked this pull request as ready for review
August 5, 2026 13:39
Collaborator
Author
|
Correction to the note above, from the final cross-PR review. I wrote "land #931 first, or reconcile the attribution call sites into it." The or is wrong: Reconciliation is mandatory, not an alternative. A reconciliation commit threading Worth stating why it matters beyond the build: if someone resolves it by loosening the signature instead, those two call sites are exactly the unkeyed span ids #931 exists to eliminate — reintroduced in brand-new code. |
The sync path derived three identifiers with bare SHA-256 and sent them to a configured endpoint. `deriveDeviceId` hashed `hostname:username` and truncated to 64 bits, commented "pseudonymous, stable". An unkeyed digest of a host and username pair is not pseudonymous against anyone who can guess plausible values: hash the guess, compare, done. `deriveSpanId` hashed the dedup key — and for pi, zerostack, lingtai-tui and codebuff that key embeds the raw absolute source path, home directory included, because the bridge passes `source.path` straight through. Guess a plausible home and project name and the same confirmation works. This is the project's own standard, not an outside opinion. Decision D1 requires a caller-supplied HMAC key for fingerprints precisely so digests of paths cannot be dictionary-attacked, and core's fingerprint module throws on an empty key to enforce it. The sync path bypassed the primitive entirely. It also contradicted the project's own user-facing guarantee: docs/sync/README.md promises that code, file contents, diffs and PATHS stay local, and the unkeyed span id shipped absolute paths (for the four providers above) in a form confirmable by anyone with a plausible guess. All three ids are now HMAC-SHA256 under the per-install privacy key — the same key core's fingerprints use — with domain prefixes so one value in two positions never yields the same digest, and composite inputs joined with the same ASCII Unit Separator (0x1f) core/fingerprint.ts uses so a value containing ':' cannot forge a field boundary. The derive functions throw on an empty key rather than degrading. The payload builder obtains the key itself, so the decode path, which runs with an empty key by design, never reaches it. Sync now REQUIRES the persisted key: privacy-key.ts exposes a strict variant that aborts the push instead of falling back to per-process randomness when the config dir is unwritable, and refuses to silently regenerate a key file that fails validation (truncated by a full disk, a partial write). Cross-process id stability is load-bearing — partially rejected batches are not ledgered precisely because deterministic span ids make full-batch retry safe — so a per-process fallback key would emit fresh ids on every retry and let the backend double-count accepted spans, and a silent re-key would orphan everything already pushed. The fingerprint consumers keep the tolerant fallback: they only need per-process stability. The refusal is now complete, and enforced for every corrupt shape: "no file at all" is the only state a first use may create. A file that exists but is unreadable, zero-byte or whitespace-only (a partial write), or fails hex validation aborts the push and is left untouched — treating those as MISSING would silently regenerate the file and re-key every id, which is exactly the case the strict path exists to refuse. First creation is also exclusive (O_CREAT|O_EXCL): when two processes race the first use, the loser re-reads and adopts the winner's key, so concurrent pushes can never mint different keys and mix cached device ids with spans derived from the other. Scope, stated honestly: sync is opt-in and needs an endpoint plus credentials, the digests are of identifiers rather than prompts or file contents, and this predates the extraction. It is not an active leak of user content. It is a weak construction the project already knows how to do properly. This change narrows the exposure rather than closing it: ai.project still ships a project name in the clear, and in one Claude fallback path that name is a dash-encoded absolute path. Blast radius: every id is re-keyed once at upgrade, so anything already pushed stops correlating with new sends and the backend sees a fresh device identity. Ids stay stable afterwards unless the key file is lost. The host-side sent ledger keys off the raw dedup key and is unaffected, so re-push filtering keeps working.
Ports three upstream commits this branch never received: the attribution feature (1bf7206), the review hardening on top of it (ccee28a), and the security follow-up that closed credential-leak paths and added session retraction (50c8251). They are ported as an end state rather than in sequence. Two and three revise one, so replaying them in order would have introduced the very issues they fix and then removed them again — and anything missed in the third pass would have shipped a feature with a reopened hole, which is the specific way this port could have gone wrong. The credential-leak paths that commit closes are enumerated and checked off individually against the result. One correction to an earlier draft of this message, which claimed no new unkeyed digest is introduced. That was wrong: stateHash in sync/otlp.ts is a new unkeyed sha256, and it feeds deriveSpanId, so it is an input to a value that goes on the wire. It is not a D1 violation — D1 governs core's fingerprint module and its caller-supplied key, while stateHash is a local ledger discriminator computed over data that is itself sent in cleartext, so it hides nothing and leaks nothing. But the sentence was false and is worth correcting rather than quietly dropping. This branch now carries getagentseal#931's commit (c467548, "fix(sync): key the device, span and trace digests") beneath this one — cherry-picked onto the shared base so the history stays two clean commits. That ordering is load-bearing: reconciliation is mandatory in every merge order, not optional. git merge-tree reports no conflict against getagentseal#931 in either direction, yet the merged file does not compile: getagentseal#931 drops the createHash import and gives the derive functions a privacyKey first argument, so an unreconciled attribution section leaves stateHash with an undefined symbol and two one-argument call sites. Rebasing replays the same breakage, which is why the earlier "land getagentseal#931 first, OR reconcile" framing was wrong. The two call sites are reconciled INTO getagentseal#931's keyed signatures, in the direction getagentseal#931 demands: buildAttributionOtlpPayload obtains the persisted host privacy key exactly as buildOtlpPayload does — one getPersistedHostPrivacyKey call per builder, no second source of the key — and threads it into deriveTraceId and deriveSpanId. This is the security point of the reconciliation: loosening the signatures back to one argument would reintroduce exactly the unkeyed span and trace ids getagentseal#931 exists to remove, in new code. stateHash stays unkeyed, deliberately: it is a local ledger discriminator over One more merge-compat fix, in getagentseal#931's own test file (sync-privacy-key.test.ts): the concurrency fixture path was built from process.cwd(), which is the repo root under `--root packages/cli` — the worker then exited on a nonexistent file before writing its ready file and the race test timed out. The path is now anchored to the test file's own location (fileURLToPath(import.meta.url)). This is the only line of getagentseal#931's tree this branch touches; 37a5b46 remains a verbatim copy of c467548. Second fix in getagentseal#931's tree, same motivation: the concurrency race test adopted with only a 50ms budget. createKeyFileExclusive polled the winner's file 5x10ms after EEXIST, and the strict entry check refused an 'invalid' file INSTANTLY — but the winner's create (open) and write are separate syscalls, and under load the loser can read the still-empty file either at entry or inside the poll. Both windows now share one bounded awaitValidKey (500ms) that ADOPTS the winner's key when it lands and otherwise throws the same refusal. Nothing is ever overwritten; a file left invalid by a crash or truncated write still fails loudly. This is the second getagentseal#931 file this branch touches; 37a5b46 remains a verbatim copy of c467548.
ozymandiashh
force-pushed
the
fix/port-sync
branch
from
August 5, 2026 17:29
616dec3 to
f29d27e
Compare
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.
Ports three upstream commits this branch never received:
1bf7206— push git attribution spans with--attribution(the feature)ccee28a— review hardening: cwd-fallback egress, Windows paths, PR-link validation50c8251— close credential-leak paths, session retraction, span/key/CLI hardeninggrep -ri attribution packages/cli/src/sync/returned nothing on this branch; upstream'spush.tshas several hits. The whole feature and its security follow-up were missing.Ported as an end state, not in sequence
Two and three revise one. Replaying them in order would have introduced the issues they fix and then removed them again — and anything missed on the third pass would have shipped a feature with a reopened security hole in the same change. That is the specific way this port could have gone wrong, so the credential-leak paths from
50c8251are enumerated and checked off individually against the result rather than assumed to arrive with the feature.Scope
packages/cli/src/sync/{cli,otlp,push}.ts,yield.ts, the mock IdP fixture, docs, changelog, plus two new test files covering the feature and its CLI surface.Nothing here touches the id derivations in
otlp.ts— those are #931's subject. No new unkeyed digest is introduced.Verification
34 new attribution tests green, 93 across the seven sibling yield/sync suites, core 509, typecheck clean in both packages. The Playwright sync E2E self-skips without its three env vars, so it stayed inert.