Skip to content

refactor: recurse the command-tree walkers - #674

Merged
axisrow merged 2 commits into
mainfrom
pr/recurse-command-tree-walkers
Aug 3, 2026
Merged

refactor: recurse the command-tree walkers#674
axisrow merged 2 commits into
mainfrom
pr/recurse-command-tree-walkers

Conversation

@axisrow

@axisrow axisrow commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Стек: базируется на #673 (pr/masters-loading-stubs). Ретаргетить на main после мержа родителя.

Три места обходят дерево команд Click ровно на два уровня и считают каждую подкоманду листом: smoke_matrix._registered_cli_commands, test_cli_contract._subcommands и независимый оракул счётчиков в test_smoke_matrix. Группа, вложенная в группу, трактовалась бы как один непрозрачный лист — её собственные подкоманды молча выпадали бы и из smoke-классификации (дыра в безопасности: DANGEROUS-команды классифицируются именно этим обходом), и из контрактных проверок CLI.

Все три обхода сделаны рекурсивными; заодно починены два потребителя, предполагавших, что путь группы не содержит точек: вызов --help теперь разбивает путь на сегменты argv, а проверка канонического имени использует rsplit (лист — всегда ПОСЛЕДНИЙ сегмент) и валидирует каждый сегмент пути по отдельности.

Чистый no-op на текущем дереве: вложенных групп сегодня нет, поэтому набор зарегистрированных команд побайтово идентичен до и после (165 команд, validate_matrix() чистый). Это подготовка, вынесенная отдельно, чтобы поведенческое изменение, вводящее первую вложенную группу, ревьюилось само по себе.

test_smoke_matrix намеренно сохраняет локальную реализацию вместо импорта из smoke_matrix: это независимый оракул для счётчиков сводки, и импорт тестируемой реализации сделал бы проверку тавтологичной.

Refs #648

🤖 Generated with Claude Code

axisrow and others added 2 commits August 3, 2026 09:38
The edit page's "Изображения" section renders in two stages:
`ImageSuggestionsEditor` appears first with four
`ImageSuggestionsEditor.CampaignContents.StubN` loading placeholders and
neither `ContentImage.*` nor `.Open` present yet, then ~3s later the stubs
are replaced by the real content.

`_wait_for_images_editor` returned as soon as the outer container existed
— i.e. during the stub window — so `_read_image_content_ids` read `[]` for
a campaign that demonstrably had 4 images, and `masters update --image`
refused to replace anything with a false "campaign has no images".

This is the same failure mode the function's own docstring already
described (the 2026-08-02 four-DRAFT regression), one render stage later:
the original guard only covered "section absent", not "section present but
still showing skeletons".

The wait now also polls until no `StubN` element remains, so every caller
observes only the settled state. The timeout message now distinguishes the
stub case ("did not finish rendering... may still be showing loading
placeholders").

Confirmed live 2026-08-03 on campaigns 713234191 and 713234204 (4 real
images each): read back as empty before the fix, as 4 after it.

Refs #648

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three places walk the Click command tree exactly two levels deep and
assume every subcommand is a leaf: `smoke_matrix._registered_cli_commands`,
`test_cli_contract._subcommands`, and `test_smoke_matrix`'s independent
count oracle. A group nested inside a group would be treated as one opaque
leaf — silently dropping its own subcommands from smoke classification
(a safety gap, since DANGEROUS commands are classified by that walk) and
from the CLI contract checks.

Makes all three recurse, and fixes the two consumers that assumed a
group path contains no dots: `--help` invocation now splits the path into
argv segments, and the canonical-name check uses `rsplit` (the leaf is
always the LAST segment) validating each path segment individually.

Pure no-op on the current tree: there are no nested groups today, so the
set of registered commands is byte-identical before and after (165
commands, `validate_matrix()` clean). This is preparation, split out so
the behavioural change that introduces the first nested group is
reviewable on its own.

`test_smoke_matrix` keeps a local re-implementation rather than importing
the one from `smoke_matrix`: it is an independent oracle for the summary's
counts, and importing the implementation under test would make it
tautological.

Refs #648

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@axisrow

axisrow commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

@claude @codex review PR #674 at exact head 8530bc8 (round 4bc5105c-9d51-4801-9b86-cf2be1e02daf). Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

@claude

claude Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @axisrow's task in 2m 17s —— View job


Review — recurse the command-tree walkers

  • Read the diff against origin/pr/masters-loading-stubs
  • Analyze the three recursive walkers for correctness
  • Check the two fixed consumers (--help argv split, rsplit canonical name)
  • Verify no-op claim / key construction equivalence
  • Post review

Verdict: LGTM — no critical issues found. The recursion is correct, the two consumer fixes are sound, and the change is behaviorally a no-op on the current (flat) tree.

What I verified

Key construction is equivalent. command_key(g, c) is exactly f"{g}.{c}" (smoke_matrix.py:240-242), so the recursive walker's f"{prefix}.{name}" produces byte-identical keys. No drift from bypassing the helper.

Group vs. leaf detection is consistent. All three walkers discriminate with hasattr(command, "commands") — the same predicate already used at the top level. A click.Group exposes .commands; a plain click.Command does not. Correct, and it terminates (Click trees are acyclic).

Iterable is imported (smoke_matrix.py:13), so the new annotation on _walk_leaf_command_keys is fine.

The --help argv split is right. group_name.split(".") + [command_name, "--help"] (test_cli_contract.py:153) expands a dotted path a.b into ["a", "b", leaf, "--help"], which is what Click needs to reach a nested leaf. The old [group_name, command_name, "--help"] would have passed "a.b" as a single argv token and failed.

The rsplit(".", 1) fix is right. The leaf is always the last segment, so rsplit correctly yields ("a.b", "leaf") for a nested command, then validates every path segment against GROUP_NAME_RE and the leaf against COMMAND_NAME_RE (test_cli_contract.py:214-224). The if "." in command guard keeps bare top-level commands out of this path. split(".", 1) would have mis-parsed a.b.leaf into ("a", "b.leaf").

Counts oracle stays consistent. test_smoke_matrix counts leaves via len(list(_walk_leaf_command_keys("", group))) (empty prefix is harmless since only the count is used), matching _registered_cli_commands()'s recursion. Keeping the local re-implementation instead of importing the impl-under-test is the correct call for an independent oracle — agreed with the PR note.

One minor, non-blocking observation

test_registered_cli_names_are_canonical (test_cli_contract.py:118-124) still validates only top-level group names (cli.commands) plus each leaf name — it does not run GROUP_NAME_RE over intermediate nested-group segments. The smoke-matrix canonical test (test_cli_contract.py:214-224) does validate every segment, so canonical enforcement isn't lost overall, but this particular test would silently skip an intermediate segment like a.B.leaf when the first nested group lands. Zero impact today (no nested groups), and arguably out of scope for a prep-only PR — flagging only so it's on record for the follow-up that introduces the first nested group.

No bugs, security, data-loss, or performance concerns. The no-op framing holds: same predicate, same key format, recursion only changes behavior once a group-in-a-group actually exists.
· pr/recurse-command-tree-walkers

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 8530bc8173

ℹ️ 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".

@axisrow
axisrow changed the base branch from pr/masters-loading-stubs to main August 3, 2026 02:53
@axisrow
axisrow merged commit 096396f into main Aug 3, 2026
6 checks passed
@axisrow
axisrow deleted the pr/recurse-command-tree-walkers branch August 3, 2026 02:55
axisrow added a commit that referenced this pull request Aug 3, 2026
Смерджено с флаки тестом, вроде он вынесен в фоллоу ап

AGENTS.md described the canonical shape as exactly `direct <group>
<command>`, which no longer matches the repo: PR #674 made three
command tree walkers recursive specifically to support nested groups,
and PR #675-#677 shipped `masters adimages get|add|delete|set` as a
real three-level leaf. The stale wording already produced a false P1
from Codex on PR #675, citing these exact lines as a contract
violation.

Reword the contract to describe a path of one or more group segments
plus a leaf command, each segment validated the same way, and add
`masters adimages` as a documented example of the existing pattern.

Closes #680

Co-authored-by: axisrow <axisrow@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant