Repository navigation
docs(adr): ADR-0033 names the SDK helper and what the fold does merge - #456
Conversation
The insertion-helper sentence in ADR-0033 was stale twice over: it named `detectors.EnsureNode`, which was deleted when bomly-sdk v0.9.0 shipped, and it said the helper "deliberately merges nothing", which stopped being true when the SDK fold started unioning scopes, locations, relationship and origins. The same paragraph already said usage facts union, so it contradicted itself. Annotated in place with a dated amendment rather than superseded: the decision the ADR records -- origins are detector-asserted, contradicting records stay distinct, no tiebreak picks a winner -- still holds. Only the helper sentence drifted. The cargo origin test carried the same stale spelling in a comment. Closes #454. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change updates ADR-0033 and a related test comment. Both now reference ChangesADR-0033 helper clarification
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to This documentation-only update introduces no runtime or deployment risk and is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bomly Diff SummaryCompared Overview
Dependency Changes✅ No dependency changes. Vulnerabilities✅ No vulnerability changes. License Changes✅ No license changes. Project Posture✅ No project posture changes ( Policy Findings✅ No policy differences were identified. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9bd9283c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ld ADR-0041 decided Codex review on #456: the amendment said a contradiction stays two nodes, which is the occurrence model ADR-0041 superseded. Two records of one identity that assert different origins are one node carrying both origins; what this ADR still owns is that no tiebreak ever picks one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Two sessions fixed issue #454 in parallel and #456 landed first. Its amendment is the better one and I kept it whole: it cites the test that proves the behaviour (TestConsolidateGraphsFoldsContradictingResolutionsKeepingBoth), pins when the helper moved (v0.9.0) against when the wrapper behaviour arrived (v0.10.0), and updates a cargo origin test alongside. My amendment is dropped rather than merged beside it -- two amendments answering one issue is the kind of thing that makes an ADR harder to read than the code it explains. What survives is the one part #456 does not cover: that "both origins are kept" is easy to over-read, because MergeOrigins does drop an entry that fails validation, an exact duplicate keyed on normalized form, and a narrowly superseded one. The supersession rule is worth stating because it is the only drop that touches a disagreement, and it is narrow by design -- two genuinely different places both survive, that being the shape of a dependency-confusion signal rather than noise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Done-criterion 1 of the SDK maturity program asks for ADR-0033's prose to be corrected; #454 records that it never was.
What was wrong (
dev-docs/adr/0033-package-origin-is-detector-asserted.md, the insertion paragraph):detectors.EnsureNode, which no longer exists; the helper isdetectorkit.EnsureNodein bomly-sdk.Graph.InsertNode, which folds by identity and unions usage facts.What this does: annotates the ADR in place with a dated amendment (the repo's existing convention, see the ADR-0041 note in the same file) instead of superseding it. The substance still holds: a gap fills, identical claims fold, and a contradiction stays two nodes with no tiebreak. The amendment states that distinction explicitly.
internal/detectors/cargo/origin_test.gocarried the same stale spelling in a comment and is fixed too.make verifypasses.Closes #454.
🤖 Generated with Claude Code
Summary by CodeRabbit