Merge remote-tracking branch 'origin/main' into worktree-20260511-165241#93
Merged
Conversation
…aliases Pre-existing tickets created before the alias feature shipped had no data.alias on their CREATE event, so reduce_ticket returned alias=None and resolve_ticket_id couldn't find them by alias — defeating the whole human-friendly identifier system for ~325 legacy tickets. This change: - _alias.py (new): deterministic alias computer mirroring ticket-alias-compute.py. 16-hex IDs get adj-noun-noun, legacy 8-hex IDs get adj-noun (one segment unavailable). - _processors.process_create: when data.alias is missing, fall back to compute_alias(ticket_id) so reduce_ticket and downstream consumers surface a stable backfilled alias. - llm_format.KEY_MAP: include 'alias' (mapped to 'a') so --format=llm carries the alias to agents that read ticket lists. - ticket-create.sh: 'Created ticket' line now leads with the alias and shows the canonical ID parenthetically. - ticket-alias-resolve.py (new): single-pass resolver — one Python process iterates all ticket dirs (was N subprocess.run calls), and also matches against backfilled aliases for legacy tickets. - ticket-lib.sh resolve_ticket_id: replaces the per-file Python loop with a single call to the new resolver. Tests: 6 new pytest cases for compute_alias parity with the shell helper and process_create backfill behaviour, plus one new resolver_alias_backfill case in test-ticket-lib.sh confirming resolve_ticket_id finds tickets by their backfilled alias. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
DSO llm-review — finding 1/6[critical] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 2/6[critical] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 3/6[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 4/6[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 5/6[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 6/6[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
…n PR #93 llm-review flagged 2 critical + 4 important findings. Addresses all six: - F1 (claimed path bug in _alias.py): rejected — the '..' count is correct (verified: _wordlist_path() resolves to plugins/dso/resources/ticket-wordlist.txt). Added test_resolver_wordlist_path_resolves_to_existing_file as the executable defense. - F2 (asymmetric fallback): fixed by single source of truth (below). The two files used to compute alias independently — _alias.py fell back to hex_id[:8] when the wordlist was missing, ticket-alias-resolve.py fell back to None. Now they cannot diverge. - F3 (DRY): ticket-alias-resolve.py now imports compute_alias from ticket_reducer._alias. Removed duplicated load_wordlist() and inline compute_alias() body. TICKET_WORDLIST_PATH env var override is honoured uniformly via _wordlist_path(). - F4 (no unit tests for resolver script): added 5 direct test cases — missing tracker dir (fails loud, exit nonzero), malformed CREATE JSON (skipped without crashing), dotfile dirs (ignored), backfilled alias resolves a legacy ticket, jira_key precedence over alias on collision. - F5 (stderr suppression hid debug info): removed `2>/dev/null` from the resolver invocation in resolve_ticket_id. The resolver now exits non-zero with a useful stderr message when the tracker dir is unreadable. - F6 (tautological stored-alias test): rewrote test_process_create_uses_ stored_alias_when_present → test_process_create_prefers_stored_over_ backfill. The new test asserts the stored value is intentionally DIFFERENT from what compute_alias would yield, so a regression where backfill clobbers the stored alias would fail the test. Test suite: 12/12 backfill pytest cases, 76/76 test-ticket-lib.sh. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| data = json.load(f).get("data", {}) or {} | ||
| stored_alias = data.get("alias") or "" | ||
| jira_key = data.get("jira_key") or "" | ||
| except (OSError, json.JSONDecodeError): |
DSO llm-review — finding 1/3[critical] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 2/3[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 3/3[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
…issing wordlist Cycle-2 review surfaced 3 new findings (1 critical, 2 important): - C1 (critical) resolver-script + python3 not validated before invocation — silent failure mode hid environment misconfig as 'no matches found'. Added explicit checks in resolve_ticket_id; both emit a clear stderr diagnostic and return 1. - I1 (important) _load() swallowed OSError silently — falling back to the 8-hex alias without ever telling the operator the wordlist was missing. Added a one-shot stderr WARN keyed on the new _WARNED_MISSING module flag, matching the shell helper's 'FALLBACK' stderr pattern. Single-print so bulk callers (resolve_ticket_id scans 18k dirs) don't log-flood. - I2 (important) test_resolver_wordlist_path_resolves_to_existing_file was redundant noise — the existing parity test already exercises the file open path implicitly. Replaced with test_load_warns_once_when_wordlist_ missing which asserts the new WARN behaviour. Test suite: 12/12 backfill pytest, 76/76 test-ticket-lib.sh. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| "override.", | ||
| file=sys.stderr, | ||
| ) | ||
| _WARNED_MISSING = True |
DSO llm-review — finding 1/1[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
… code Cycle-3 review: process-substitution piping (`done < <(python3 ...)`) discards the resolver's exit code. If the resolver crashes mid-scan or exits non-zero for environment reasons, the read loop completes silently and resolve_ticket_id reports 'no matches' — indistinguishable from 'ticket genuinely not found'. - Capture stdout into a variable and check $? separately. Non-zero resolver exit now produces 'Error: alias resolver exited N for input X' on stderr and propagates as exit 1 from resolve_ticket_id. - Guard the read loop against the implicit trailing newline in <<<"$var" so empty resolver output doesn't iterate once with empty fields. - New pytest test_resolver_nonzero_exit_propagates_to_resolve_ticket_id forces the failure path (tracker-as-file → resolver OSError → exit 1) and asserts resolve_ticket_id surfaces the diagnostic. Test suite: 13/13 backfill pytest, 76/76 test-ticket-lib.sh. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DSO llm-review — finding 1/5[critical] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 2/5[critical] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 3/5[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 4/5[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 5/5[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
…in ticket_create
Cycle-4 review surfaced 5 findings. Two criticals (F1, F2) are defended in a
separate PR comment (see GitHubPRDefenseStore); they rest on misreadings of the
flow. The three remaining findings are addressed here:
- F3 (important): ticket-alias-compute.py was unguarded against 8-hex inputs
(would have crashed on int('', 16) had it ever been called with one).
Added len(hex_id) >= 12 guard mirroring ticket_reducer._alias. Dead-code-
safe today (callers only pass 16-hex) but defends against future drift.
- F4 (important): no test verified the new alias-led 'Created ticket' output.
Updated tests/scripts/test-ticket-create.sh Test 1 to accept both formats
and to assert the alias-led variant appears whenever the bundled wordlist
is available. Also fixed a latent parity gap — ticket-lib-api.sh
ticket_create had its own duplicate output line which still printed the
legacy 'Created ticket <id>: <title>' format; ticket-create.sh shell-path
was already updated, but the lib-api in-process path was not. Now both
paths emit the alias-led summary identically.
- F5 (important): tightened test_resolver_nonzero_exit_propagates_to_resolve_
ticket_id. The earlier OR'd assertion could pass on a silent crash (the
'success-line not in stdout' branch was true vacuously). Replaced with
two AND'd assertions: explicit diagnostic in stderr, success-line not in
stdout.
Also: review.max_resolution_attempts lowered from 5 to 3 in dso-config.conf
per session decision — three autonomous fix/defend cycles is the cap before
user escalation; more than that is churn.
Test suite: 13/13 backfill pytest, 76/76 test-ticket-lib.sh, 17/17 test-
ticket-lib-api.sh, 48/48 test-ticket-create.sh.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
Defenses — cycle-4 findings F1 + F2The cycle-4 review surfaced two critical findings that I am defending rather than fixing, because both rest on misreadings of the code/flow. Cycle-4 findings F3, F4, F5 are fixed in commit F1 —
|
DSO llm-review — finding 1/4[critical] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 2/4[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 3/4[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
DSO llm-review — finding 4/4[important] Posted by dso_ci_review.runner; resolve this comment when addressed. |
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.
Auto-generated PR for branch
worktree-20260511-165241(created by merge-to-main-pr.sh).