Repository navigation
refactor: share test and demo command setup - #2322
codeforester merged 1 commit into
Conversation
codeforester
left a comment
codeforester
left a comment
There was a problem hiding this comment.
Automated review findings (note: reviewed after merge — flagging for a follow-up if these matter)
-
Confirmed regression - history context no longer set on failed resolve (
cli/bash/commands/basectl/subcommands/project_command_helpers.sh,base_project_command_resolve_context): in the pre-refactor code (bothdemo.shandtest.shonmainat2ab3e09c),base_project_set_history_contextran before the four-field validation check that can triggerbase_std_fatal_error. In the new sharedbase_project_command_resolve_context, the order is reversed: the validation check now runs first, andbase_project_set_history_contextis only reached if it passes. I verified this directly against both versions of the source. Concretely: runningbasectl demo myproject(orbasectl test myproject) against a project that resolves fine (name/root/manifest all populate) but has no demo script/test command configured used to still exportBASE_CLI_HISTORY_PROJECT/_PROJECT_ROOT/_MANIFESTbefore exiting with the fatal error; now it exits without ever setting them. Anything relying on that history context being populated even on this specific failure path (e.g. shell history/audit tooling) will silently stop seeing it. -
Wrapper executable check gap (
test.sh, the--test-preflightresolve call): the-xcheck on$BASE_HOME/bin/base-wrappernow only guards the first wrapper invocation (insidebase_project_command_resolve_context); the second, hand-rolled preflight call re-deriveswrapperwith no accompanying check. Low severity in practice — a separate trust check earlier in the same call path happens to catch a missing/non-executable wrapper first — but it's a real gap if that mitigation ever moves. -
Wrapper path still hardcoded in 3 places:
$BASE_HOME/bin/base-wrapperis independently declared inbase_project_require_manifest_command_trust, the newbase_project_command_resolve_context, and inline intest.sh's preflight call — despite this PR's purpose being to deduplicate exactly this kind of shared logic.
Posted via Claude Code
…2328) ## Summary - restore resolved history context before validating the required test/demo action - add regression coverage for missing test commands and demo scripts ## Issue Fixes #2324 Follow-up to the merged PR #2322 review finding `5256545518`. ## Validation - `bats cli/bash/commands/basectl/tests/project-command-helpers.bats cli/bash/commands/basectl/tests/test.bats cli/bash/commands/basectl/tests/demo.bats` (45 passed) - `git diff --check` - ShellCheck on changed files ## Notes The fatal error and nonzero behavior remain unchanged; only the ordering of context capture is restored.
## Summary - centralize the project-command wrapper path and executable guard - validate the wrapper immediately before the test preflight invocation - add focused coverage for a missing or non-executable wrapper ## Issue Fixes #2325 Follow-up to the merged PR #2322 review findings `5256545518`. ## Validation - `bats cli/bash/commands/basectl/tests/project-command-helpers.bats cli/bash/commands/basectl/tests/test.bats cli/bash/commands/basectl/tests/manifest-command-trust.bats` (37 passed) - `git diff --check` - ShellCheck on changed files ## Notes The change is limited to the shared project-command path; unrelated Base wrapper call sites and manifest trust policy are unchanged.
1 participant
Summary
project_command_helpers.sh.Validation
git diff --check.AI-context docs are unchanged because this is an internal refactor with no new user-facing contract.
Fixes #2297