Repository navigation
enhancement: clarify trust status reasons and help - #2648
codeforester merged 2 commits into
Conversation
codeforester
left a comment
codeforester
left a comment
There was a problem hiding this comment.
Review of #2648 (head a45a2d1, base 6fdc5a0)
Verdict: correct and safe to merge once the section-order question below is settled; no blocking defects. All CI checks are green. I did not run the suites locally; I read the full diff and checked it against main.
What I verified
- The new reason labels work as the issue asked.
Approval stale: manifestis 24 characters andApproval stale: test reqsis 25. At width 60 with a 40-character project name the PROJECT column shrinks to 24 and both labels survive intact, so the new test exercises the real renderer path. Only the display strings changed; reason codes, JSON/YAML/CSV/TSV and trust decisions are untouched. - The help text is accurate: the workspace-root fallback (
--workspace, thenworkspace.root, then the parent ofBASE_HOME), "scan" only for status discovery, "resolve the project" for allow/revoke/require, and no third copy of the trust-scope text (it points tobasectl trust status <project>). - Python help now uses two shared constants, and the internal
requiresubcommand got the resolution wording. This was one of my earlier notes on #2647. - The old exact-line pins in
help.batswere updated, and the CHANGELOG entry is present.
Please look at (inline comments carry the details)
- Help section order breaks the repo convention. All four help pages now put
Purpose:beforeUsage:. 17 otherbasectlsubcommand files use Usage then Purpose, and #2647 asked only to move the parent summary ahead of Usage. Moving the three leaves goes beyond the issue and makestrustthe odd one out. - Two spellings of the same fallback.
BASE_HOME's parent(status leaf and Python help) versusthe parent of BASE_HOME(parent, allow, revoke, docs). The tests pin both forms, so the split is now locked in. - The "does not say scan" assertions are close to vacuous. They check that the old exact string
Workspace directory to scanis absent, but the replacement text is different. A regression to "Workspace to scan" would pass. The Python parametrized test has no absence check at all. - Ten new full-line
grep -Fqxpins on wrapped prose in.ai-context/COMMANDS.md. Any re-wrap breaks them. The new help test already shows a better pattern (join and squeeze whitespace, then match a phrase). - Nits:
Approval staleis new vocabulary. The detailed output and the sharedblocked_command_summarysay "manifest command or declared test requirements contract changed". It fits the length budget, so I'm fine with it, butdocs/manifest-command-trust.mdcould define it in one sentence. In the parent help, "instead of using its positional argument" is ambiguous, because "its" has three possible commands.
Housekeeping: neither #2646, #2647 nor this PR has a milestone. If this is meant to land before #2642 (v1.11.0, due 2026-11-07), set it there and add the blocked-by link so the ordering stated in the Notes is enforced.
| case "$trust_command" in | ||
| status) | ||
| cat <<'EOF' | ||
| Purpose: |
There was a problem hiding this comment.
Convention: Purpose: now comes before Usage: here and in the allow and revoke leaves (lines 62 and 83) and in the parent (line 8). The other 17 basectl subcommand files that have a Purpose section put Usage: first (for example ci.sh, doctor.sh, gh_issue.sh). #2647 only asked to move the parent summary ahead of Usage. I'd keep Usage first in the three leaves, and for the parent either keep the issue's requested order or accept it as a parent-only exception. Update the new help test accordingly if you move them (it does not depend on order today, since it flattens the text).
| --workspace <path> Workspace directory to scan. | ||
| --project <name> Select a project explicitly instead of using [project]. | ||
| --workspace <path> Workspace to scan for command-bearing projects. | ||
| Defaults to workspace.root, then BASE_HOME's parent. |
There was a problem hiding this comment.
Wording split: this line and the Python help say BASE_HOME's parent, while the parent help (line 25), allow (73), revoke (93), .ai-context/COMMANDS.md and docs/command-reference.md say the parent of BASE_HOME. Pick one phrasing. the parent of BASE_HOME reads better, but then Python _STATUS_WORKSPACE_HELP / _PROJECT_WORKSPACE_HELP and both pinned assertions in help.bats (status leaf and trust --help) and test_command_error_paths.py change too.
| Options: | ||
| --project <name> Select a project explicitly, including one named help. | ||
| --workspace <path> Workspace directory to scan. | ||
| --project <name> Select a project explicitly instead of using |
There was a problem hiding this comment.
Nit: in the parent usage, instead of using its positional argument has no clear antecedent, because the block lists three commands. instead of the positional <project> argument would be unambiguous, and it matches what the leaves say.
| help_text="$(tr '\n' ' ' <<<"$output" | tr -s ' ')" | ||
| [[ "$help_text" == *"Record local approval for the current manifest command contract"* ]] | ||
| [[ "$help_text" == *"Workspace used to resolve the project"* ]] | ||
| [[ "$help_text" != *"Workspace directory to scan"* ]] |
There was a problem hiding this comment.
This absence check only catches the exact old string Workspace directory to scan. The new help says Workspace used to resolve the project, so a regression to Workspace to scan or Workspace directory to scan for ... (note the latter still contains the old string, but the former does not) would pass. Assert [[ "$help_text" != *scan* ]] for the allow and revoke leaves; neither leaf has a legitimate use of the word. The same gap exists in test_trust_python_help_describes_workspace_resolution: it only asserts the positive phrase, so add assert "scan" not in help_text for require, allow and revoke.
| grep -Fqx -- "- \`basectl trust status [project]\` - inspect one project's manifest command" "$commands_file" | ||
| grep -Fqx -- "- \`basectl trust <allow|revoke> <project>\` - add or remove local approval for" "$commands_file" | ||
| grep -Fqx -- "- \`basectl trust status [project]\` - read-only inspection of local trust. With" "$commands_file" | ||
| grep -Fqx -- " no project, show a compact, blocked-first summary of discovered" "$commands_file" |
There was a problem hiding this comment.
These ten full-line grep -Fqx pins make the wrapped prose in .ai-context/COMMANDS.md a test contract, so any reflow fails the suite. The new help test above already has a sturdier idiom: tr '\n' ' ' | tr -s ' ' and then match a phrase. Use that on the trust section (or on the whole file) and assert the key phrases: read-only inspection, compact, blocked-first summary, workspace.root, allow records, revoke removes.
| tmp_path: Path, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| capsys: pytest.CaptureFixture[str], | ||
| ) -> None: |
There was a problem hiding this comment.
Worth a one-line comment here explaining the budget: at width 60 the 25-character label leaves zero slack (24 + 7 + 25 + 2 gaps of 2 = 60). That is a good thing, since it makes the test fail if the label grows, but a future editor will not know the width and the 40-character name are chosen on purpose.
Final review (head cd8a0b4): looks good, ready to mergeI read the incremental diff (a45a2d1..cd8a0b4) against my six inline comments and re-checked the PR metadata.
No further changes requested. One note for #2642: its new reason codes should follow the |
Summary
basectl trustparent and leaf help, including workspace discovery versus project resolution.Issues
Closes #2646
Closes #2647
Validation
./bin/base-test: full Python suite (1,845 passed) and Bats suite (1,074 passed).help.batsafter the final wording assertion: 27 passed.shellcheck -s bash cli/bash/commands/basectl/subcommands/trust.shgit diff --checkDemo Impact
No demo changes; this updates trust command output and help.
Docs Impact
Updated the command reference, manifest-command trust guide, and changelog.
AI Context Impact
Updated
.ai-context/COMMANDS.mdto match the command behavior and help.Notes
This PR closes both issues as a documented multi-issue exception: both are small fixes to the same trust CLI surface and share the help, docs, changelog, and regression-test changes. Landing them together avoids overlapping edits to the same help text. Both issues and this PR are assigned to v1.11.0, and #2642 is blocked by both issues.