feat(sync): push git attribution spans, with the hardening that followed - #932
feat(sync): push git attribution spans, with the hardening that followed#932ozymandiashh wants to merge 1 commit into
Conversation
|
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. |
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. That matters for merge order. getagentseal#931 rewrites the id derivations in this same file, and this commit adds a new digest plus two new consumers of deriveSpanId and deriveTraceId that getagentseal#931 was not written against. Land getagentseal#931 first, or reconcile the attribution call sites into it.
3527dcf to
616dec3
Compare
|
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. |
1 participant
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.