qualogy-solutions/jira-ticket-analysis

Read-only snapshot · generated 2026-09-30T06:49:10.884Z · act on PRs directly in Bitbucket · ← all repos · auto-merge is off for this repo — reviews still post automatically, merging is manual

✅ Auto-merged (2)

test: validate QRAD-247 multi-tenancy
#7 by Deborah · feature/QRAD-247 → main · Open in Bitbucket →
low

This PR only adds new test cases to two existing test files, validating multi-tenancy isolation in the orchestrator and tenant resolver. No production code is modified, so it cannot break runtime behavior, though it could reveal existing bugs in CI.

  • Changes confined entirely to test files (tests/test_orchestrator_temporal.py, tests/test_worker_tenancy.py)
  • No modifications to production/business logic, infra, auth, or data layers
  • Purely additive (327 lines added, 0 removed) with no deletions or refactors of existing tests
  • Worst case impact is a CI failure surfacing a real multi-tenancy bug, not a production incident

Test-only PR adding solid multi-tenancy coverage for QRAD-247 (two tenants through one worker, independent caching, TTL expiry, unconfigured tenant isolation). Tests are well documented, use distinct fixtures per tenant to avoid false positives, and align with existing patterns in the test suite. No correctness, security, or production-code issues found.

No inline comments.

QRAD-248: strengthen analysis grounding and format enrichment timestamp
#5 by Deborah · feature/QRAD-248-e2e-fixes → main · Open in Bitbucket →
low

Changes a timestamp formatting helper for Jira enrichment headers (presentation-only, non-idempotency-affecting) and strengthens/expands the LLM system prompt's anti-hallucination rule, with accompanying test coverage. No auth, payments, schema, or infra changes; behavior change is confined to prompt wording and a display string.

  • <item>Timestamp change is explicitly presentation-only; docstring confirms it doesn't affect idempotency/marker detection logic</item> <item>Prompt change only tightens instructions to the LLM to avoid hallucinated tech claims, not a functional/business-logic change</item> <item>Well-covered by new and updated tests (test_jira_adapter.py, test_analyse_prompt.py)</item> <item>Small diff confined to two source files plus their tests, no touches to auth, payments, DB schema, or infra</item> </reasons>

Change is low-risk: a presentation-only timestamp formatting helper (well tested) and a prompt-wording strengthening with accompanying tests. No correctness or security issues found; only minor stylistic/readability notes.

core/orchestrator.py:236 · info
Wrapping the prompt string in parentheses with an inline comment block between two triple-quoted string literals works (Python implicitly concatenates adjacent string literals), but it's a somewhat unusual pattern that could confuse readers into thinking the comment is part of the string. Consider moving the QRAD-257 rationale comment above the `_ANALYSE_SYSTEM_PROMPT = (` line instead of inside the concatenation.
connectors/jira_adapter.py:42 · info
Truncating to minute precision means two enrichment runs within the same minute produce an identical header string. Since idempotency is based on the marker prefix (not this timestamp) this is safe per the docstring, but worth double-checking that no other code path (e.g. audit/history diffing) relies on this timestamp being unique per run.

🗄️ Closed elsewhere (merged/declined outside this tool) (4)

feat(QRAD-286): redesign approval UI
#8 by Deborah · feature/QRAD-286-admin-ui-redesign → main · Open in Bitbucket →
medium

This is a UI/view-model redesign of the tenant admin approvals page (card layout, pipeline step summarization, new CSS) plus an additive, optional 'created' field on the webhook_bridge PausedWorkflow API response. It does not touch approval decision logic, auth checks, or the actual approve/reject actions, but it is on the admin approvals path and does change a public API response model (backward-compatible) and the internal PausedWorkflow model used by tenant_admin.

  • <item>New pure-function module (approval_view.py) is well-tested with extensive unit tests covering token parsing edge cases</item> <item>Changes touch tenant_admin/main.py's approvals_list route, replacing 'paused' context var with 'rows' -- template/context contract change confined to one page</item> <item>webhook_bridge/routes/admin.py adds a new optional field to a public API response model (PausedWorkflow) -- low risk since additive/backward-compatible, but it's a public API contract touched, which raises caution</item> <item>No changes to authentication/authorization, approve/reject decision logic, database schemas, or payment/billing code</item> <item>Diff is fairly large (633 additions) but mostly template/CSS/tests, not sprawling across unrelated systems</item> <item>Affects an admin-facing approval workflow UI; a bug could confuse admins reviewing pending approvals but does not directly enable unauthorized actions</item> </reasons>

Well-structured refactor with good test coverage for the new pure view-model module. Only minor issues found; nothing blocking.

tenant_admin/approval_view.py:38 · info
jira_already_fetched is computed from a plain `in` check on the whole list; if source:jira_ticket=already-fetched ever appears without a preceding 'fetched' token, the description branch for 'fetched' would never run and the token is silently skipped by the `continue` below, losing the info entirely for that run. Not currently possible given the producer's token order, but worth a defensive note/assert given the module's stated goal of never inventing/losing info from a token that is present.
tenant_admin/approval_view.py:137 · info
format_created only strips a trailing 'Z'; fromisoformat pre-3.11 doesn't accept other non-`+HH:MM` offsets or fractional seconds with more precision variants some Temporal/JSON serializers may emit, but the except ValueError fallback (return created) handles that path gracefully, so this is low risk.
tenant_admin/approval_view.py:161 · info
build_approval_row uses wf['workflow_id'], wf['tenant_id'], wf['ticket_id'] with bracket access (KeyError on missing keys) while `created` and `events` use .get(). Given PausedWorkflow guarantees these are required fields this is fine, but the inconsistency could surprise a future caller passing a partial dict.
webhook_bridge/routes/admin.py:118 · info
wf.start_time.isoformat() will omit an explicit UTC designator/offset if the Temporal SDK returns a naive datetime, which would then be misinterpreted by approval_view.format_created's tz-aware parsing path (fromisoformat succeeds but dt is naive, hour/minute still render fine since no timezone conversion is performed, so likely low impact) -- worth confirming start_time is always timezone-aware.
QRAD-248: add anti-hallucination and formatting rules to analyse prompt
#4 by Deborah · feature/QRAD-248 → main · Open in Bitbucket →
low

This PR only adds two additional guidance rules (anti-hallucination and markdown formatting) to a system prompt string used in the analyse step, plus new unit tests pinning prompt wording. No control flow, schema, API, or data handling logic is changed.

  • Change is confined to a prompt string in core/orchestrator.py plus a new test file", "No changes to business logic, schemas, APIs, auth, or infra", "Small diff size (94 lines added, 0 removed)", "Worst-case impact is degraded LLM prompt behavior, not a functional break", "Well covered by new regression tests for the prompt content"]}

Small, low-risk prompt-text addition plus dedicated regression tests. The new rules read clearly and the tests are well scoped to string-builder functions with no external dependencies. No correctness or security issues found; one minor suggestion below.

core/orchestrator.py:236 · info
Rule 6 is a good addition, but consider explicitly mentioning that this also applies to ticket_type-driven framing text (e.g. not asserting UI frameworks/tools based on ticket wording alone) to make the scope unambiguous to the model, since 'ticket text itself' is later allowed as evidence which could be read as licensing exactly the kind of inference this rule is meant to forbid.
QRAD-288: add local RAG E2E validation evidence
#9 by Deborah · feature/QRAD-288 → main · Open in Bitbucket →
low

This PR adds only new, explicitly local-only test/dev-infrastructure files (embedding provider stub, local activities substitute, worker wiring, and several E2E/smoke test files) plus corresponding tests. No production files (worker.py, rag_indexing/*, connectors/*, llm_provider.py) are modified, and the new files are documented as not tracked/deployed and guarded against accidental use against real Key Vault/production DSNs.

  • All changed/added files are net-new (1713 additions, 0 deletions) and confined to local-dev/testing infrastructure, not production code paths.
  • Extensive docstrings and guard logic (vault URL checks, DSN checks, tenant checks) explicitly prevent this code from running against real Azure Key Vault, production DB, or production tenants.
  • No modification to authentication, billing, database schema/migrations, or production API contracts.
  • New tests include explicit checks (hash-based) verifying that key production files (activities.py, worker.py, workflow.py, db.py, bitbucket_skills.py, worker_tenancy.py) remain unmodified.
  • Credentials/secrets handling is read from local .env.local only, never logged, and isolated from the shared os.environ used by other tests.
  • Even though the diff is sizeable (1713 lines), it is additive test/dev tooling rather than sprawling changes across unrelated production areas.

This PR adds a large set of local-only, git-untracked developer tooling and tests (Ollama embedding provider shim, local RAG activities substitute, and several live/manual smoke tests) for QRAD-288. The code is heavily documented, guarded against accidental production use (vault-URL checks, tenant checks, presence-only logging), and the tests look thorough. Only a couple of minor, non-blocking issues found; nothing that should block merge given this is explicitly local-only, non-production, non-deployed tooling.

local_ollama_embedding_provider.py:100 · info
response.json() is called without a try/except for JSONDecodeError. If Ollama returns a non-JSON 200 response (e.g. an HTML error page from a proxy), this will raise an unguarded exception instead of the clear RuntimeError pattern used elsewhere in this method. Minor since this is local-only tooling, but worth wrapping for consistency with the other error paths.
rag_indexing_activities_local.py:120 · info
install() guards against AZURE_KEY_VAULT_URL containing 'vault.azure.net', but if AZURE_KEY_VAULT_URL is unset/empty this check is silently skipped and the patch installs anyway. That's presumably the intended default for pure local use, but worth double-checking there isn't a scenario where an unset var actually means "use default production vault" elsewhere in the codebase, which would bypass this safety net.
tests/test_step7_semantic_retrieval_live.py:66 · info
The DSN safety check only verifies the marker '127.0.0.1:55432' is a substring of RAG_INDEX_DB_URL; a maliciously or accidentally crafted DSN (e.g. one with that substring in a password or path) could pass this naive check. Low risk given this is a local-only, skip-by-default test file, but the same pattern is repeated across multiple new test files and could be centralized/hardened if it starts being trusted as a real safety gate.
Fix guardrail dangerous_instructions finding suppression in orchestrator
#6 by Deborah · feature/QRAD-267 → main · Open in Bitbucket →
high

This PR fixes a security-relevant bug in the orchestrator where dangerous_instructions guardrail findings were computed but silently discarded, causing potentially harmful tickets to bypass human approval. It changes core authorization/guardrail merge logic that gates whether human review is required, affecting the safety-critical approval pipeline for all ticket processing.

  • Touches core/orchestrator.py's guardrail evaluation and resolve_execution_authorization call path, which determines whether human approval is bypassed
  • Changes security-sensitive fail-closed behavior (ADR-6 part 5) around a classifier used to detect dangerous/harmful instructions
  • Previously silent finding-suppression bug being fixed here means behavior change could newly trigger (or fail to trigger) human approval gates across all tenants
  • Modifies shared data models (ContentGuardrailFinding -> GuardrailFinding) and merges findings into a unified guardrail_result used by multiple call sites
  • Mistakes here could either cause false negatives (dangerous content slipping through) or over-blocking (excessive human approval gates), both high impact for a safety/security control

This is a well-reasoned bugfix: it properly merges the dangerous_instructions classifier findings into the same guardrail_result instead of discarding them, and replaces the "None guardrail_result" fail-closed hack with an explicit failed_checks entry that is now surfaced in checks_run/authorization_reason. Tests are updated accordingly and add a new regression test for the previously-silent finding-suppression bug. No correctness, security, or edge-case issues found in the diff.

No inline comments.