[worktree-20260605-175523] fix(acli): explicit client-cred access in verify-created-issue (PR #685 review)#686
Merged
Conversation
…689) _verify_created_issue read JIRA_URL/JIRA_USER/JIRA_API_TOKEN from os.environ, so create-path tests that mocked only subprocess.run silently switched to the urllib REST GET whenever ambient Jira credentials were present in the dev/CI process. Pin the credential source to the explicit AcliClient: read client.jira_url/user/api_token (None-safe), drop the os.environ read, and fall back to the subprocess get_issue path when no client is supplied. Thread client through create_issue -> _create_issue_from_json -> _verify_created_issue so the priority (--from-json) path is covered too. Production behaviour is preserved (AcliClient.create_issue forwards client=self, whose creds come from the env at construction). Adds two RED->GREEN tests; no regression (31 pass). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…a666/b38a/38fd/4cc1) test_e2e_dedup_pass::test_pre_existing_dso_id_produces_zero_creates asserted an obsolete pre-4354 contract: it wired an *inbound* create yet expected *outbound* applier-dedup artifacts (mapping.json + a dedup-create-skipped manifest event). That applier guard never existed on the inbound-create path, and the get_comments AttributeError the tickets cited was a caught symptom, not the cause. Since commit 1f0032d (bug 4354), dedup of already-bound issues is the snapshot differ's job: it recognises the dso-id:<local_id> label and stands down (no inbound CREATE, no phantom local ticket, no ghost label write-back). Rewrite the test to that production-correct end-to-end contract (label-bound issue -> zero creates via differ stand-down -> apply() no-op) plus a regression guard that genuinely-unbound issues still create. No production change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cher dedup test (4cc1) test_fetcher_dedup_observable::test_dedup_suppression_emits_alert passed alone but failed in the full suite: it patched plugins.dso.scripts.dso_reconciler.alert_store.append directly, while fetcher._load_alert_store() resolves that same dotted sys.modules key at call time. test_bridge_alerts_surface registers a DIFFERENT module object under the key, so when it ran first the patched object and the object the fetcher used diverged and the patch was silently missed (captured []). Patch the _load_alert_store SEAM instead (order-independent), and add an autouse fixture that snapshots/clears the shared dotted key around each test. Closes the second half of 4cc1 (the first half is the dedup-pass test rewrite). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DSO-Story: worktree-20260605-175523
… review) Address PR #685 llm-review finding 1: replace getattr(client, attr, '') or '' with direct client.<attr> access (guarded by 'if client is not None'). AcliClient always sets jira_url/user/api_token in __init__, so direct access is safe and surfaces a malformed (non-None but partial) client as an explicit AttributeError rather than silently degrading to the subprocess path with a half-populated credential set. No behavioural change for the supported callers (AcliClient or None); 31 acli tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR coordinates credential sourcing improvements in ACLI integration with test enhancements across the DSO workflow reconciliation pipeline. The core change pins REST verification credentials to explicit client objects rather than ambient environment variables, adds test coverage validating this behavior, refactors dedup validation to focus on differ-level semantics, hardens alert observability tests with module isolation, and increments the plugin version. ChangesDSO Workflow Credential Pinning and Dedup Test Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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
2 participants
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.
Commits
Auto-generated by merge-to-main-pr.sh from
git log --no-merges origin/main..HEAD.Summary by CodeRabbit
Chores
Bug Fixes