Clean up workBenches base and devBench tooling#1
Conversation
Reviewer's GuideRefactors base and devBench AI/spec tooling by moving spec-kit/OpenSpec into the dev-bench base layer with reusable Speckit worktree helpers, adds SonarQube/SonarScanner tooling and SonarCloud coverage helpers, introduces robust yolo/ct shell functions, and extends project detection and docs for pyBench/phpBench while tightening docker/devcontainer reconciliation and AI provider configuration. Sequence diagram for Speckit worktree and AI helper workflowsequenceDiagram
actor Dev
participant speckit_worktree_enable
participant git_feature as create-new-feature.sh
participant git_worktree as git_worktree
participant feature_json as feature.json
participant ct as ct_functions
participant select_wt as select-worktree.sh
participant claude_cli as claude
Dev->>speckit_worktree_enable: run speckit-worktree-enable
speckit_worktree_enable->>feature_json: install git extension config
speckit_worktree_enable->>ct: install ct, cta, ctc, ctg, cts
Dev->>speckit_worktree_enable: run /speckit.specify
speckit_worktree_enable->>git_feature: invoke speckit-git-feature hook
git_feature->>git_worktree: git worktree add -b <branch>
git_feature-->>speckit_worktree_enable: BRANCH_NAME, WORKTREE_PATH
speckit_worktree_enable->>feature_json: write feature_directory for spec
Dev->>ct: cta
ct->>select_wt: select-worktree.sh --path
select_wt-->>ct: WORKTREE_PATH
ct->>claude_cli: start claude in tmux at WORKTREE_PATH
claude_cli-->>Dev: interactive AI session in feature worktree
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@codex review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The Speckit git/worktree templates include project- and user-specific values (e.g.,
worktree_root: ../ledgerlinc-model-ocr-pipeline-worktreesingit-config.ymland the hardcoded/home/brett/...path inct.zsh); consider replacing these with generic defaults or relative paths so generated projects are not tied to a specific environment. - There is a fair amount of duplicated logic between the project-local worktree helpers (e.g.,
.specify/shell/worktrees.sh) and the globalct-functions.zsh; it may be worth centralizing common behavior into a single shared script to reduce drift and simplify future changes. - In
reconcile-devcontainer-container.sh, failing hard when the expected image is missing (exit 1) might be surprising in multi-bench workflows; consider treating this as a non-fatal condition (log and exit 0) so callers can decide whether a missing image should stop their pipeline.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Speckit git/worktree templates include project- and user-specific values (e.g., `worktree_root: ../ledgerlinc-model-ocr-pipeline-worktrees` in `git-config.yml` and the hardcoded `/home/brett/...` path in `ct.zsh`); consider replacing these with generic defaults or relative paths so generated projects are not tied to a specific environment.
- There is a fair amount of duplicated logic between the project-local worktree helpers (e.g., `.specify/shell/worktrees.sh`) and the global `ct-functions.zsh`; it may be worth centralizing common behavior into a single shared script to reduce drift and simplify future changes.
- In `reconcile-devcontainer-container.sh`, failing hard when the expected image is missing (`exit 1`) might be surprising in multi-bench workflows; consider treating this as a non-fatal condition (log and exit 0) so callers can decide whether a missing image should stop their pipeline.
## Individual Comments
### Comment 1
<location path="devBenches/base-image/files/speckit-worktree/templates/specify/shell/worktrees.sh" line_range="10-13" />
<code_context>
+
+# Auto-detect repo root from this script's own location so the file is
+# relocatable and does not embed a host-specific absolute path.
+LEDGERLINC_SPECKIT_REPO_ROOT="$(CDPATH="" cd "${${(%):-%x}:A:h}/../.." 2>/dev/null && pwd)"
+if [ -z "$LEDGERLINC_SPECKIT_REPO_ROOT" ]; then
+ # bash fallback when the zsh prompt expansion above is unavailable
+ LEDGERLINC_SPECKIT_REPO_ROOT="$(CDPATH="" cd "$(dirname "${BASH_SOURCE[0]:-$0}")/../.." 2>/dev/null && pwd)"
+fi
+LEDGERLINC_SPECKIT_LAST_WORKTREE_SCRIPT="$LEDGERLINC_SPECKIT_REPO_ROOT/.specify/extensions/git/scripts/bash/get-last-worktree.sh"
</code_context>
<issue_to_address>
**issue (bug_risk):** Sourcing this script in bash will fail due to unguarded zsh-specific parameter expansion.
Because the script is advertised as source-able from both bash and zsh, the unguarded `${${(%):-%x}:A:h}` causes `bad substitution` in bash before the fallback can run. Please gate the zsh-only expansion on `$ZSH_VERSION`, for example:
```bash
if [ -n "${ZSH_VERSION:-}" ]; then
LEDGERLINC_SPECKIT_REPO_ROOT="$(CDPATH="" cd "${${(%):-%x}:A:h}/../.." 2>/dev/null && pwd)"
else
LEDGERLINC_SPECKIT_REPO_ROOT="$(CDPATH="" cd "$(dirname "${BASH_SOURCE[0]:-$0}")/../.." 2>/dev/null && pwd)"
fi
```
</issue_to_address>
### Comment 2
<location path="devBenches/goBench/README.md" line_range="140-141" />
<code_context>
# Go shortcuts
# (alias examples - these would be configured in shell)
go run, go build, go test, go mod, etc.
+go-sonar-coverage # go test coverage.out + sonar-scanner
# Docker & Kubernetes
</code_context>
<issue_to_address>
**issue (typo):** The documented SonarCloud coverage helper uses two different command names (`go-sonar-coverage` vs `sonarcloud-go-coverage`).
Alias examples use `go-sonar-coverage`, while the dedicated section uses `sonarcloud-go-coverage`. Please either standardize on one name or clarify if they are different tools to avoid user confusion.
```suggestion
go run, go build, go test, go mod, etc.
sonarcloud-go-coverage # go test coverage.out + sonar-scanner
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR updates the shared workBench/devBench base tooling, moves spec-driven tooling into the devBench base layer, adds SonarQube/SonarCloud helpers, removes Grok from supported AI tooling, and refreshes bench inventory from pythonBench to pyBench with added phpBench support.
Changes:
- Reworks base/devBench AI and spec CLI installation, yolo/ct helpers, and OpenCode plugin handling.
- Adds Speckit worktree tooling, SonarQube MCP startup, and Java/Go/C++ SonarCloud coverage helpers.
- Updates bench detection/config/docs/tests for
pyBench,phpBench, Grok removal, and new devBench tooling.
Reviewed changes
Copilot reviewed 87 out of 88 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
.gitignore |
Updates ignored bench directories and Python artifacts. |
README.md |
Refreshes image layer and bench inventory documentation. |
base-image/Dockerfile |
Updates Layer 0 version, removes spec CLI ownership, and adds yolo function. |
base-image/files/opencode/opencode.json |
Removes pinned OpenCode auth plugin versions. |
base-image/install-ai-clis.sh |
Updates AI CLI install flow, adds Antigravity, removes Grok/OpenSpec. |
bench-config.json.backup |
Renames Python bench backup entry to pyBench. |
config/bench-config.json |
Renames Python bench config and adds phpBench config. |
config/shell/zshrc |
Converts yolo alias into a tmux-backed function. |
devBenches/.devcontainer/devcontainer.json |
Starts SonarQube MCP for the devBenches workspace. |
devBenches/README.md |
Documents phpBench/pyBench and shared Sonar tools. |
devBenches/base-image/Dockerfile |
Adds spec-driven tools, ct helpers, and worktree bootstrap into Layer 1a. |
devBenches/base-image/files/ct/ct-functions.zsh |
Adds global Speckit worktree launcher functions. |
devBenches/base-image/files/opencode/opencode.json |
Removes pinned OpenCode auth plugin versions. |
devBenches/base-image/files/openspeckit/setup-openspeckit |
Adds repo bootstrapper for shared OpenSpec/Speckit agent context. |
devBenches/base-image/files/speckit-worktree/speckit-worktree-enable |
Adds Speckit worktree workflow bootstrap command. |
devBenches/base-image/files/speckit-worktree/templates/agents/skills/speckit-git-feature/SKILL.md |
Adds shared agent skill for Speckit git feature creation. |
devBenches/base-image/files/speckit-worktree/templates/claude/skills/speckit-git-feature/SKILL.md |
Adds Claude skill for Speckit git feature workflow. |
devBenches/base-image/files/speckit-worktree/templates/claude/skills/speckit-specify/SKILL.md |
Adds Claude skill for Speckit specify workflow. |
devBenches/base-image/files/speckit-worktree/templates/codex/skills/speckit-git-feature/SKILL.md |
Adds Codex wrapper skill for Speckit git feature workflow. |
devBenches/base-image/files/speckit-worktree/templates/codex/skills/speckit-git-feature/agents/openai.yaml |
Adds Codex agent metadata for git feature skill. |
devBenches/base-image/files/speckit-worktree/templates/codex/skills/speckit-specify/SKILL.md |
Adds Codex wrapper skill for Speckit specify workflow. |
devBenches/base-image/files/speckit-worktree/templates/codex/skills/speckit-specify/agents/openai.yaml |
Adds Codex agent metadata for specify skill. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/README.md |
Documents the bundled Speckit git extension. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/commands/speckit.git.commit.md |
Adds Speckit auto-commit command prompt. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/commands/speckit.git.feature.md |
Adds Speckit feature checkout command prompt. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/commands/speckit.git.initialize.md |
Adds Speckit git initialize command prompt. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/commands/speckit.git.remote.md |
Adds Speckit remote detection command prompt. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/commands/speckit.git.validate.md |
Adds Speckit branch validation command prompt. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/config-template.yml |
Adds generic git extension config template. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/extension.yml |
Defines bundled Speckit git extension hooks and commands. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/git-config.yml |
Adds default git extension config. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/scripts/bash/auto-commit.sh |
Adds Bash auto-commit helper. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/scripts/bash/get-last-worktree.sh |
Adds Bash last-worktree lookup helper. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/scripts/bash/git-common.sh |
Adds Bash git utility functions. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/scripts/bash/initialize-repo.sh |
Adds Bash git initialization helper. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/scripts/powershell/auto-commit.ps1 |
Adds PowerShell auto-commit helper. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/scripts/powershell/git-common.ps1 |
Adds PowerShell git utility functions. |
devBenches/base-image/files/speckit-worktree/templates/specify/extensions/git/scripts/powershell/initialize-repo.ps1 |
Adds PowerShell git initialization helper. |
devBenches/base-image/files/speckit-worktree/templates/specify/shell/ct.zsh |
Adds per-repo ct wrapper template. |
devBenches/base-image/files/speckit-worktree/templates/specify/shell/select-worktree.sh |
Adds worktree selector helper. |
devBenches/base-image/files/speckit-worktree/templates/specify/shell/worktrees.sh |
Adds per-repo fallback worktree helpers. |
devBenches/base-image/install-ai-clis.sh |
Aligns devBench AI CLI install docs/logic with Grok removal and OpenCode upstream. |
devBenches/base-image/install-testing-tools.sh |
Adds Sonar tools and bumps testing utility versions. |
devBenches/cppBench/.devcontainer/devcontainer.json |
Chains Sonar MCP, Layer 3 ensure, and stale-container reconciliation. |
devBenches/cppBench/Dockerfile.layer2 |
Adds C++ coverage tooling and Sonar helper alias. |
devBenches/cppBench/README.md |
Documents C++ SonarCloud coverage workflow. |
devBenches/devcontainer.test/README.md |
Documents expanded devBench base test coverage. |
devBenches/devcontainer.test/test-ct-launchers.sh |
Adds ct launcher behavior tests. |
devBenches/devcontainer.test/test.sh |
Expands Layer 1a tests for spec, ct, Sonar, and PATH tooling. |
devBenches/goBench/.devcontainer/devcontainer.json |
Chains Sonar MCP, Layer 3 ensure, and stale-container reconciliation. |
devBenches/goBench/Dockerfile.layer2 |
Adds Go SonarCloud coverage helper and alias. |
devBenches/goBench/README.md |
Documents Go SonarCloud coverage workflow and pyBench naming. |
devBenches/goBench/files/sonarcloud-go-coverage |
Adds Go coverage and Sonar scanner wrapper. |
devBenches/javaBench/.devcontainer/devcontainer.json |
Chains Sonar MCP, Layer 3 ensure, and stale-container reconciliation. |
devBenches/javaBench/Dockerfile.layer2 |
Adds JaCoCo/SonarCloud helpers and aliases. |
devBenches/javaBench/README.md |
Documents Java SonarCloud coverage workflows. |
devBenches/pythonBench/.devcontainer/devcontainer.json |
Removes legacy pythonBench devcontainer. |
devBenches/pythonBench/.devcontainer/docker-compose.yml |
Removes legacy pythonBench compose file. |
devBenches/pythonBench/.env |
Removes legacy pythonBench environment file. |
devBenches/pythonBench/Dockerfile.layer2 |
Removes legacy pythonBench Layer 2 image. |
devBenches/pythonBench/scripts/build-layer.sh |
Removes legacy pythonBench build wrapper. |
devBenches/pythonBench/setup.sh |
Removes legacy pythonBench setup script. |
devBenches/scripts/ai-cli-adapter.sh |
Removes Grok support and filters unsupported configured providers. |
devBenches/scripts/ensure-sonarqube-mcp.sh |
Adds reusable SonarQube MCP/proxy startup script. |
devBenches/scripts/launchDevBench |
Adds PHP bench description/detection/help text. |
devBenches/scripts/update-devBench-project.sh |
Adds PHP project detection and pyBench naming. |
devBenches/scripts/workspace-type-detector.sh |
Adds PHP workspace indicators and TUI option. |
devcontainer.test/test.sh |
Updates yolo test for function implementation. |
docker-compose.mounts.yml |
Removes Grok mount and updates AI/spec CLI mount reference. |
docs/CONTAINER-ARCHITECTURE.md |
Updates Layer 0/1a tool ownership documentation. |
docs/MOUNTS-README.md |
Updates standard mounts, Grok removal, and spec CLI ownership docs. |
docs/newBench.md |
Updates new bench mount template with .agents and no Grok. |
docs/spec-driven-development.md |
Clarifies spec-driven tools are for developer benches. |
scripts/AI-PROVIDER-SETUP.md |
Removes Grok from provider documentation. |
scripts/configure-ai-priority.sh |
Removes Grok and filters unsupported providers from saved priority config. |
scripts/ensure-layer3.sh |
Reworks Layer 3 user/group validation without running containers. |
scripts/interactive-setup.sh |
Updates default bench list to pyBench. |
scripts/launchBench |
Adds PHP bench description. |
scripts/metadata-helper.sh |
Adds PHP sibling detection and pyBench naming. |
scripts/new-project.sh |
Updates AI prompt examples to pyBench. |
scripts/reconcile-devcontainer-container.sh |
Adds stale devcontainer reconciliation helper. |
scripts/setup-ui/src/utils/config.ts |
Updates UI default benches to pyBench. |
scripts/update-bench-config.sh |
Updates bench descriptions for pyBench and phpBench. |
scripts/update-project.sh |
Adds PHP indicators and updates prompt examples to pyBench. |
sysBenches/devcontainer.test/test.sh |
Updates yolo test for function implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21cdc35cae
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 87 out of 88 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
devBenches/cppBench/.devcontainer/devcontainer.json:90
- Grok has been removed from the supported provider list and the standard mount documentation in this PR, but this bench still keeps the Grok credential mount. Remove this stale mount and comment so the devcontainer matches the supported AI CLI inventory and doesn't require an unused host path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 373a7e8f58
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 437a5b1831
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 90 out of 91 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
devBenches/cppBench/.devcontainer/devcontainer.json:90
- Grok has been removed from the installer, provider priority, adapter, and canonical mount docs, but this devcontainer still retains the Grok credential mount/comment. Remove the stale Grok mount here so the bench matches the supported AI provider set and does not keep depending on a deprecated credential path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7c8457178
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
brettheap
left a comment
brettheap
left a comment
There was a problem hiding this comment.
AI-Assisted Swarm Code Review
This review was produced by an automated multi-agent (swarm) review process. Each finding was adversarially verified before posting. Please apply human judgment before acting on any item.
Severity counts: 2 high, 4 medium, 19 low (26 total).
Inline comments posted: 25 of 25 findings mapped to diff lines.
All findings mapped successfully to diff lines; none fell back to this summary.
| - Dependencies on other packages (use `addBlockedBy` if needed) | ||
|
|
||
| ### Spawn agents | ||
| Use the **Agent tool** to spawn one `general-purpose` agent per work package **in a single message** (parallel launch). Each agent prompt must include: |
There was a problem hiding this comment.
[high] Phase 3 mixes the stateful team protocol with fire-and-forget Agent (Task) subagents
Phase 3 spawns teammates with the wrong mechanism, turning all team-coordination steps into no-ops.
Phase 3 builds the full team subsystem: TeamCreate (line 98), TaskCreate with addBlockedBy (lines 101-105), then requires the spawned agents to claim a task ID (line 111), report back when blocked mid-run (lines 116, 124), be coordinated across waves (lines 125-127), and finally receive a shutdown_request and TeamDelete (lines 131-132).
But line 108 spawns the workers with the Agent tool as plain general-purpose subagents. A plain Agent subagent has no team membership, no shared-task-list access, and communicates only by its return value. So it cannot claim a TaskCreate task ID, cannot stream a blocker while running, and cannot receive a shutdown_request. As written, the model creates a team it never populates with real teammates, and the wave coordination, blocker reporting, and shutdown steps all become no-ops.
Fix: spawn teammates via the team mechanism, e.g. Task({ team_name: "apply-<change>", name: "<package>", subagent_type: "general-purpose", run_in_background: true }). Alternatively, if fire-and-forget subagents are intended, drop TeamCreate/TaskCreate/addBlockedBy/shutdown_request/TeamDelete plus the claim/report-blocker instructions, and have the lead assign non-overlapping file groups up front and reconcile each batch's returned results after it completes. Pick one model and use it consistently.
| test_tool_output "Graphite CLI" "gt --version" | ||
| test_tool "Sonar env helper" "command -v sonar-env" | ||
| test_tool "Sonar env helper configures keychain" "sonar-env --check | grep -q 'SONARQUBE_CLI_KEYCHAIN_FILE=.*sonarqube-cli/keychain.json'" | ||
| test_tool "Sonar env helper loads token aliases" "tmpdir=\$(mktemp -d); printf 'SONARQUBE_TOKEN=test-token\nSONARQUBE_ORG=test-org\n' > \"\$tmpdir/sonar.env\"; SONARQUBE_ENV_FILE=\"\$tmpdir/sonar.env\" sonar-env -- sh -c 'test \"\$SONAR_TOKEN\" = test-token && test \"\$SONAR_ORGANIZATION\" = test-org && test \"\$SONARQUBE_CLI_TOKEN\" = test-token && test \"\$SONARQUBE_CLI_ORG\" = test-org'; status=\$?; rm -rf \"\$tmpdir\"; exit \$status" |
There was a problem hiding this comment.
[high] exit $status inside eval'd test command aborts the entire test run early
Line 99 (the new "Sonar env helper loads token aliases" test) passes a command string ending in ...; status=$?; rm -rf "$tmpdir"; exit $status to test_tool, which evaluates it at line 28 as if eval "$command" > /dev/null 2>&1; then. An if condition does not spawn a subshell and eval does not fork, so the trailing exit $status runs in the top-level test.sh process and terminates the entire script.
Because this diff also replaced set -e with set -uo pipefail and changed both helpers to return 0 on failure, nothing else aborts the run — so this exit is the single thing that does.
Impact:
- Every assertion after line 99 (lines 100-129) is silently skipped.
- The final pass/fail summary never prints.
- When
sonar-envsucceeds,status=0, so the script exits0— a green run that actually skipped ~30 assertions, giving false confidence.
The neighbouring multi-statement tests wrap their bodies in bash -lc/bash -c/zsh -ic/sonar-env -- sh -c, so any exit inside them only ends that child process — which is why this one is uniquely broken.
Fix: keep cleanup local to a child process or subshell; no exit may run in the eval'd shell. Run the body in a ( ... ) subshell, or wrap in bash -c '...' with a trap ... EXIT for cleanup like the sibling tests.
|
|
||
| **Proposal updates:** [None / Updated sections X, Y] | ||
|
|
||
| **Next step:** Run `/opsx:explore` or `/opsx:apply` — clarifications.md will be available as context for design and task generation. |
There was a problem hiding this comment.
[medium] clarify points the user at /opsx:apply for design/task generation, which apply does not do
The clarify summary's next-step line (line 225) and the IMPORTANT note (line 11) both direct the user to /opsx:apply for design/task generation. But commands/opsx/apply.md does not generate design.md or tasks.md. Its Phase 1 runs openspec instructions apply, and on missing artifacts (state: "blocked") it explicitly tells the user to run /opsx:propose (apply.md lines 35-36). Phases 2-5 only implement existing tasks from tasks.md.
The command that actually generates the remaining artifacts from the proposal + clarifications is commands/opsx/propose.md Phase 6 (propose.md lines 354-356, 365), which reads clarifications.md to produce design.md and tasks.md.
Impact: a user who runs /opsx:clarify before design/tasks exist and follows this guidance to /opsx:apply will hit the blocked state and be bounced to /opsx:propose anyway.
Fix: point the next step (and the line-11 note) at /opsx:propose (Phase 6 generates design.md and tasks.md using clarifications.md as context), then /opsx:apply to implement.
| local encoded | ||
|
|
||
| encoded="-${path#/}" | ||
| encoded="${encoded//\//-}" |
There was a problem hiding this comment.
[medium] Transcript path encoding omits '.' -> '-' substitution used by Claude Code
encoded_project_dir() (line 51) builds ~/.claude/projects/<encoded> by replacing only / with - (encoded="${encoded//\//-}"). Claude Code actually encodes dots as well as slashes to -. Confirmed on this machine: /workspace/projects/Metalx/metalx-fence-estimator/.worktrees/001-map-search-foundation is stored as ...-estimator--worktrees-001-map-search-foundation (the . in .worktrees became the second -). The slash-only encoding here yields ...-estimator-.worktrees-001-..., which does not exist.
Since this subsystem targets Speckit worktrees that live under a dotted .worktrees/ directory, any such project root makes [ -d "$project_dir" ] fail, so latest_transcript returns nothing and latest_command/last_prompt_summary silently fall back to fragile pane-snapshot heuristics — the dashboard loses its real command/prompt source for exactly the worktrees it is meant to track.
Fix: also collapse . (and _ for safety) to -:
| encoded="${encoded//\//-}" | |
| encoded="-${path#/}" | |
| encoded="${encoded//[\/._]/-}" |
| fi | ||
|
|
||
| local raw_value | ||
| raw_value=$(awk -F':' -v key="$key" '$1 == key {sub(/^[^:]*:[[:space:]]*/, "", $0); print $0; exit}' "$config_file") |
There was a problem hiding this comment.
[medium] Config parser does not strip inline comments, unlike the sibling parser
resolve_config_value (new file, line 32) parses values with awk and then only trims surrounding whitespace via trim. It does not strip a trailing # comment or surrounding quotes. The sibling parser get_config_value in create-new-feature.sh (line 282) reads the same .specify/extensions/git/git-config.yml and deliberately strips both via sed -E "s/[[:space:]]+#.*$//; ...; s/^['\"]//; s/['\"]$//".
The keys read by resolve_config_value are base_branch (used as a ref name) and worktree_root (used as a path). The shipped git-config.yml is comment-heavy, so a user adding base_branch: main # team default is plausible — that yields main # team default here, diverging from create-new-feature.sh which resolves it to main. The two parsers should agree.
| raw_value=$(awk -F':' -v key="$key" '$1 == key {sub(/^[^:]*:[[:space:]]*/, "", $0); print $0; exit}' "$config_file") | |
| raw_value=$(awk -F':' -v key="$key" '$1 == key {sub(/^[^:]*:[[:space:]]*/, "", $0); print $0; exit}' "$config_file") | |
| raw_value=$(printf '%s' "${raw_value:-}" | sed -E "s/[[:space:]]+#.*$//; s/^['\"]//; s/['\"]$//") | |
| raw_value=$(trim "$raw_value") |
| - ✅ Spec-driven tools (`specify`, `openspec`) | ||
| - ✅ Worktree-mode Speckit bootstrap installation | ||
| - ✅ Global `ct*` helper availability in interactive zsh | ||
| - ✅ Clean failure outside non-Speckit directories |
There was a problem hiding this comment.
[low] Double-negative description contradicts the test it documents
Line 50 (added in this diff) reads - ✅ Clean failure outside non-Speckit directories. The corresponding test in test.sh runs ctlist from a temp dir that is NOT a Speckit repo and asserts it errors out — i.e. it validates clean failure IN a non-Speckit directory. The phrase "outside non-Speckit directories" is a double negative that reads as "inside Speckit directories," the opposite of the behavior tested.
| - ✅ Clean failure outside non-Speckit directories | |
| - ✅ Clean failure in non-Speckit directories |
| sonarcloud-java-gradle | ||
| ``` | ||
|
|
||
| This runs `test`, `jacocoTestReport`, and `sonarqube` using `./gradlew` when it |
There was a problem hiding this comment.
[low] README says gradle helper runs sonarqube task, but the script runs sonar
The README's newly added line says the Gradle helper runs test, jacocoTestReport, and sonarqube. But files/sonarcloud-java-gradle:53 invokes the sonar task (exec "${GRADLE_CMD[@]}" test jacocoTestReport "${SONAR_GRADLE_TASK:-sonar}" "$@"). The README pins the SonarQube Gradle plugin at 7.3.0.8198; as of plugin 5.x+ the task was renamed from sonarqube to sonar, and the old name is deprecated/removed. The doc is stale and would mislead a user who tries gradle sonarqube.
| This runs `test`, `jacocoTestReport`, and `sonarqube` using `./gradlew` when it | |
| This runs `test`, `jacocoTestReport`, and `sonar` using `./gradlew` when it |
| - SSH keys (`.ssh`) | ||
| - GitHub CLI (`.config/gh`) | ||
| - Grok credentials (`.grok`) | ||
| - Copilot credentials (`.copilot-cli`) |
There was a problem hiding this comment.
[low] Sonar read-only mount missing from Read-Only Mounts list
This diff adds the Sonar credential mount (source=${localEnv:HOME}/.config/sonarqube,...,readonly) to both the Standard Mount Set and the AI Credential Summary, and edits this same hunk's Read-Only Mounts list (lines 166-170) by removing the Grok entry. However it does not add Sonar here, even though every other readonly credential mount (Copilot, gh, ssh, gitconfig) is listed. For a file that bills itself as "the canonical reference," the readonly inventory is now incomplete.
| - Copilot credentials (`.copilot-cli`) | |
| - Copilot credentials (`.copilot-cli`) | |
| - Sonar tokens (`.config/sonarqube`) | |
| - Shell configs (`.zshrc`, `.oh-my-zsh`, `.p10k.zsh`, `.bashrc`) |
|
|
||
| ### Cached Mounts | ||
| AI tool credentials use `consistency=cached` for performance — frequently accessed, rarely modified: | ||
| - Shared agent workflow (`.agents/`) |
There was a problem hiding this comment.
[low] .pi/ cached mount omitted from Cached Mounts list
This diff adds ~/.pi as a consistency=cached mount in the AI Credential Mounts JSONC block and lists it as cached in the AI Credential Summary, but the Security Notes -> Cached Mounts list (line 174) only adds .agents/ and omits .pi/. This makes the canonical mounts reference internally inconsistent.
| - Shared agent workflow (`.agents/`) | |
| - Shared agent workflow (`.agents/`) | |
| - Project Intelligence (`.pi/`) | |
| - Claude (`.claude/`, `.claude.json`) |
| │ ├── javaBench/ ← Java bench (opensoft/javaBench) | ||
| │ └── pythonBench/ ← Python bench (opensoft/pythonBench) | ||
| │ ├── phpBench/ ← PHP bench (opensoft/phpBench) | ||
| │ └── pyBench/ ← Python bench |
There was a problem hiding this comment.
[low] pyBench tree entry omits the (opensoft/pyBench) repo annotation
In the directory-tree diagram, every devBench entry carries its repo annotation (e.g. phpBench/ ← PHP bench (opensoft/phpBench)), but the renamed pyBench/ entry on line 114 was written as pyBench/ ← Python bench with no (opensoft/pyBench). The Repositories table (line 193 in this same diff) confirms pyBench maps to opensoft/pyBench, so the omission is an inconsistency introduced when renaming pythonBench→pyBench.
| │ └── pyBench/ ← Python bench | |
| │ └── pyBench/ ← Python bench (opensoft/pyBench) |
brettheap
left a comment
brettheap
left a comment
There was a problem hiding this comment.
🤖 AI Swarm Code Review — pass 3 (net-new findings)
A third swarm pass; most findings re-confirmed earlier reviews (the swarm is converging). These 7 are not yet covered by existing comments (1 high · 1 medium · 5 low).
Notable:
opsx/apply.md:108— Phase 3 builds the stateful team protocol (TeamCreate/TaskCreate/addBlockedBy/shutdown_request/TeamDelete) but spawns workers via the fire-and-forget Agent tool, so the coordination/blocker/shutdown steps are no-ops. Pick one model.
Advisory; severities are the swarm's estimate. Generated with Claude Code.
| - Dependencies on other packages (use `addBlockedBy` if needed) | ||
|
|
||
| ### Spawn agents | ||
| Use the **Agent tool** to spawn one `general-purpose` agent per work package **in a single message** (parallel launch). Each agent prompt must include: |
There was a problem hiding this comment.
[high] Phase 3 spawns teammates with the wrong mechanism, turning all team-coordination steps into no-ops.
Phase 3 builds the full team subsystem: TeamCreate (line 98), TaskCreate with addBlockedBy (lines 101-105), then requires the spawned agents to claim a task ID (line 111), report back when blocked mid-run (lines 116, 124), be coordinated across waves (lines 125-127), and finally receive a shutdown_request and TeamDelete (lines 131-132).
But line 108 spawns the workers with the Agent tool as plain general-purpose subagents:
Use the Agent tool to spawn one
general-purposeagent per work package ...
This is the wrong spawn method. The repo's own orchestrating-swarms skill (~/.claude/skills/orchestrating-swarms/SKILL.md) documents two distinct mechanisms:
| Aspect | Task/Agent subagent | Task + team_name + name (teammate) |
|---|---|---|
| Lifespan | Until task complete | Until shutdown requested |
| Communication | Return value | Inbox messages |
| Task access | None | Shared task list |
| Team membership | No | Yes |
A plain Agent subagent has no team membership, no shared-task-list access, and communicates only by its return value. So it cannot claim a TaskCreate task ID, cannot stream a blocker while running, and cannot receive a shutdown_request. As written, the model creates a team it never populates with real teammates, and the wave coordination, blocker reporting, and shutdown steps all become no-ops.
Fix: spawn teammates via the team mechanism, e.g. Task({ team_name: "apply-<change>", name: "<package>", subagent_type: "general-purpose", run_in_background: true }), and replace "Use the Agent tool" on line 108 accordingly. Alternatively, if fire-and-forget subagents are actually intended, drop TeamCreate/TaskCreate/addBlockedBy/shutdown_request/TeamDelete plus the "claim task ID / report blocker mid-run" instructions, and have the lead assign non-overlapping file groups up front and reconcile each batch's returned results after it completes. Pick one model and use it consistently.
| local encoded | ||
|
|
||
| encoded="-${path#/}" | ||
| encoded="${encoded//\//-}" |
There was a problem hiding this comment.
[medium] encoded_project_dir() (line 51) builds ~/.claude/projects/<encoded> by replacing only / with -:
encoded="-${path#/}"
encoded="${encoded//\//-}"Claude Code actually encodes dots as well as slashes to -. Confirmed directly on this machine: the worktree path /workspace/projects/Metalx/metalx-fence-estimator/.worktrees/001-map-search-foundation is stored as:
~/.claude/projects/-workspace-projects-Metalx-metalx-fence-estimator--worktrees-001-map-search-foundation
The . in .worktrees became the second -, producing the --. The slash-only encoding here would instead yield ...-estimator-.worktrees-001-..., which does not exist.
Since this subsystem targets Speckit worktrees that live under a dotted .worktrees/ directory, any such project root makes [ -d "$project_dir" ] fail, so latest_transcript returns nothing and latest_command / last_prompt_summary silently fall back to fragile pane-snapshot heuristics — the dashboard loses its real command/prompt source for exactly the worktrees it is meant to track.
Fix: also collapse . to -. Claude Code also remaps _, so include it for safety:
| encoded="${encoded//\//-}" | |
| encoded="-${path#/}" | |
| encoded="${encoded//[\/._]/-}" |
| return 1 | ||
| } | ||
|
|
||
| tmux set-option -g mouse on >/dev/null 2>&1 || true |
There was a problem hiding this comment.
[low] config/shell/zshrc:672 sets mouse mode globally on the tmux server:
tmux set-option -g mouse on >/dev/null 2>&1 || trueThe -g flag mutates the running server's global config for every session, not just the freshly created yolo-* session. A user who keeps a long-lived tmux server and deliberately runs with mouse mode off (e.g. to allow native terminal text selection) will have that preference silently overwritten the first time they invoke yolo, and it persists after the yolo session exits.
Since $session_name is already in scope (set on line 659 and used by the immediately following attach-session -t "$session_name"), scope the option to that session:
| tmux set-option -g mouse on >/dev/null 2>&1 || true | |
| tmux set-option -t "$session_name" mouse on >/dev/null 2>&1 || true |
Low severity (guarded with || true, cosmetic setting), but it is a real, surprising cross-session behavioral change.
|
|
||
| ## Phase 2: Assemble the Audit Team | ||
|
|
||
| Create a team and spawn **4 specialist agents in parallel**. Each agent runs specific analysis passes and reports findings back to you. |
There was a problem hiding this comment.
[low] Phase 2 says "Create a team and spawn 4 specialist agents in parallel" (line 52), and the final guardrail says "send shutdown requests to all team agents and clean up the team" (line 375). But the only spawning mechanism specified is the Agent tool with subagent_type: general-purpose and run_in_background: false (line 76) — the Task subagent system, which does not create a managed team object (no TeamCreate is invoked anywhere in this file). Agent/Task subagents auto-terminate when they return their result, so there is no team to shut down or delete.
This leaves "create a team" / "clean up the team" as instructions with no corresponding tool action, which may confuse the executing model (it could hunt for a TeamDelete-style step that doesn't apply). The same "clean up the team" wording appears in opsx-clarify (line 238). Low severity since the orphaned cleanup step is effectively a no-op, but the wording should either drop the team-creation/cleanup language in favor of plain parallel Agent subagents, or actually use the team subsystem (TeamCreate/TeamDelete) consistently.
| candidates.append( | ||
| Path.home() | ||
| / "projects" | ||
| / "workBenches" |
There was a problem hiding this comment.
[low] worktree_template_root() appends a fallback candidate hardcoded to one developer's checkout layout:
candidates.append(
Path.home()
/ "projects"
/ "workBenches"
/ "devBenches"
/ "base-image"
/ "files"
/ "speckit-worktree"
/ "templates"
)This only resolves for a checkout that happens to live at ~/projects/workBenches; for any other location it is inert dead weight. More importantly, it is redundant: the preceding script-relative candidate script_path.parent.parent / "speckit-worktree" / "templates" already resolves to the exact same path generically (the script lives at .../files/openspeckit/setup-openspeckit, so parent.parent is .../files). The hardcoded branch therefore adds no portable value.
Note the practical impact is limited: a missing candidate is simply skipped, so this line alone never triggers the downstream SystemExit("Speckit worktree template root not found.") — that only fires when every candidate (env var, /usr/local/share/..., script-relative) misses. Recommend removing the hardcoded block and relying on the documented SPECKIT_WORKTREE_TEMPLATE_ROOT env var, the install path, and the script-relative candidate.
| export SONARQUBE_CLI_DIR="${XDG_CACHE_HOME:-$HOME/.cache}/sonarqube-cli" | ||
| fi | ||
|
|
||
| mkdir -p "$SONARQUBE_CLI_DIR" 2>/dev/null || true |
There was a problem hiding this comment.
[low] SONARQUBE_CLI_DIR (${XDG_CACHE_HOME:-$HOME/.cache}/sonarqube-cli, line 14) backs the SonarQube CLI file keychain (keychain.json, line 20), which stores an auth token. It is created with:
mkdir -p "$SONARQUBE_CLI_DIR" 2>/dev/null || trueWith a default umask of 022 this directory is created mode 0755 (group/world readable and traversable). On a shared CI runner or multi-user host, other accounts could then read the keychain file once the CLI writes the token. Since this directory exists specifically to hold a secret, lock it down to the owner:
| mkdir -p "$SONARQUBE_CLI_DIR" 2>/dev/null || true | |
| mkdir -p "$SONARQUBE_CLI_DIR" 2>/dev/null || true | |
| chmod 700 "$SONARQUBE_CLI_DIR" 2>/dev/null || true |
Severity is low: it sits under the per-user $HOME (often already 0700/0750), and the token is also present in environment variables regardless, so this is defense-in-depth rather than a direct exposure. Note: the original finding cited lines 893/896, but the file is only 79 lines long — the actual lines are 17 (mkdir) and 20 (keychain).
| ``` | ||
|
|
||
| #### SonarCloud Coverage | ||
| `sonarcloud-cpp-gcovr` reads `SONARQUBE_TOKEN` from |
There was a problem hiding this comment.
[low] The newly-added doc line states the script "reads SONARQUBE_TOKEN from ~/.config/sonarqube/sonar.env", but devBenches/cppBench/files/sonarcloud-cpp-gcovr actually:
- Accepts
SONAR_TOKENdirectly;SONARQUBE_TOKENis only a fallback that populatesSONAR_TOKEN(lines 16-17, 29). - Honors
SONARQUBE_ENV_FILEto override the secrets path (line 4). - Falls back to
~/.config/ledgerlinc/secrets/sonar.envif the default path is absent (lines 5-6).
A user who has only SONAR_TOKEN exported would, per the README, wrongly think they must set SONARQUBE_TOKEN. Suggest wording it as reads SONAR_TOKEN/SONARQUBE_TOKEN from ~/.config/sonarqube/sonar.env (override with SONARQUBE_ENV_FILE). Low impact since the script's own error message already lists both variable names.
|
|
||
| # Keep only providers that are still supported. | ||
| for provider in "${configured_priority[@]}"; do | ||
| if printf '%s\n' "${DEFAULT_PROVIDERS[@]}" | grep -q "^${provider}$"; then |
| "source=${localEnv:HOME}/.notebooklm-mcp-cli,target=/home/${localEnv:USER}/.notebooklm-mcp-cli,type=bind,consistency=cached" | ||
| "source=${localEnv:HOME}/.notebooklm-mcp-cli,target=/home/${localEnv:USER}/.notebooklm-mcp-cli,type=bind,consistency=cached", | ||
| // SonarCloud / SonarQube tokens for scanners and MCP integration | ||
| "source=${localEnv:HOME}/.config/sonarqube,target=/home/${localEnv:USER}/.config/sonarqube,type=bind,readonly" |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6613d3efd2
ℹ️ 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".
| cat > "$path" <<'EOF' | ||
| installed: [] |
There was a problem hiding this comment.
When speckit-worktree-enable runs in a repo that has .specify/ but no existing .specify/extensions.yml, this fresh-file path writes installed: [] while adding a speckit.git.feature hook that depends on the git extension. In that scenario the script reports the worktree workflow as enabled, but Spec Kit has not actually enabled/registered the git extension, so /speckit.specify can skip the feature/worktree hook; the generated file should include git in installed or run the normal extension registration first.
Useful? React with 👍 / 👎.
| if [ "${GIT_CONFIG_EXISTED:-false}" = true ] && [ "$force" != true ] && [ "${GIT_OVERLAY_FORCE:-false}" != true ]; then | ||
| echo "kept existing $path (use --force to replace)" | ||
| return 0 |
There was a problem hiding this comment.
When users rerun speckit-worktree-enable --base-branch ... --worktree-root ... after the workflow is already installed, this early return keeps the old git-config.yml and ignores the newly parsed options, even though the command's success text prints the requested values. That leaves subsequent feature creation using the stale base branch/worktree root unless users know to use destructive --force; update the top-level config values instead of returning here.
Useful? React with 👍 / 👎.
| - Global agent entrypoint: `{agent_root / "AGENTS.md"}` | ||
| - Workflow protocol: `{agent_root / "protocols" / "openspec-speckit-workflow.md"}` | ||
| - Bootstrap contract: `{agent_root / "protocols" / "project-agent-bootstrap.md"}` |
There was a problem hiding this comment.
On a fresh devBench host where ~/.agents has only been created by the new devcontainer initializeCommand, setup-openspeckit writes repo-local agent files that instruct every agent to read these global protocol files, but this script never creates ~/.agents/AGENTS.md or the protocols/*.md targets. The result is a successfully bootstrapped repo whose authoritative workflow links are dead, so agents lose the OpenSpec/Speckit protocol unless the user happened to pre-populate those files; seed the global files or fall back to repo-local copies before inserting these pointers.
Useful? React with 👍 / 👎.
brettheap
left a comment
brettheap
left a comment
There was a problem hiding this comment.
🤖 AI Swarm Code Review — pass 4 (multi-pass run, net-new)
3-pass deep run (308 agents). Most findings re-confirmed earlier reviews; these 6 are not yet on the PR (1 medium · 5 low).
Advisory; severities are the swarm's estimate. Generated with Claude Code.
| aarch64|arm64) sonar_platform="linux-arm64" ;; \ | ||
| *) echo "Unsupported architecture for SonarQube CLI: $arch" >&2; exit 1 ;; \ | ||
| esac; \ | ||
| if ! command -v sonar >/dev/null 2>&1; then \ |
There was a problem hiding this comment.
[medium] The guard if ! command -v sonar defeats the stated purpose of this block. Confirmed against the repo:
- Line 146 runs
install-testing-tools.sh, which at its lines 233-234 already downloadssonarto/usr/local/bin/sonar(pinned toSONARQUBE_CLI_VERSIONdefault0.10.0.1266). That installer is best-effort: itslog_erroronly prints to stderr and does not exit, so a partial/failed download still lets the build continue. - Therefore, in the normal path
command -v sonarsucceeds and the guardedcurl --retry 5 --retry-delay 10 ...download in this "explicit, required, hardened" block never runs. The block degrades to a baresonar --version.
Consequences:
- The retry/robustness this block exists to provide is bypassed whenever the best-effort installer placed any runnable
sonar— possibly at a different version than this block's ARG. The twoSONARQUBE_CLI_VERSIONdefaults are maintained in separate files (Dockerfile ARG vs the installer script) and can drift, so the Dockerfile's pinned version is not authoritatively enforced. - Under
set -eux, the final unguardedsonar --versionaborts the build if the best-effort installer left a broken/partial binary — and the retried re-download that could have repaired it is exactly what the guard skips.
Fix: drop the guard so Layer 1a is the single source of truth. Download unconditionally (e.g. to a temp path), verify --version, then move into place — making this block authoritative for both the binary and the pinned version.
| - Always prompt for change selection if not provided | ||
| - Use artifact graph (openspec status --json) for completion checking | ||
| - Don't block archive on warnings - just inform and confirm | ||
| - Preserve .openspec.yaml when moving to archive (it moves with the directory) |
There was a problem hiding this comment.
[low] Confirmed. Line 154 (added in this new file) reads:
- Preserve .openspec.yaml when moving to archive (it moves with the directory)
A repo-wide search finds .openspec.yaml referenced only on this line (plus its mirror copy under base-image/files/claude/commands/opsx/archive.md). Every other opsx command — propose.md lines 66, 104, 131, 207, 219, 241, 263 — names the project config openspec/config.yaml. No openspec tooling produces a per-change .openspec.yaml marker file.
The guardrail is internally inconsistent either way:
- Name mismatch — if this is meant to be the project config, it should read
openspec/config.yaml. - The parenthetical is misleading — the archive step runs
mv openspec/changes/<name> openspec/changes/archive/..., so only files insideopenspec/changes/<name>/move with the directory.openspec/config.yamllives outside the change directory and would not move, so "it moves with the directory" is false for the config file.
Since no .openspec.yaml file exists, an agent following this guardrail could waste effort looking for a nonexistent file. Recommend either removing the guardrail or, if a real per-change file needs preserving, naming it correctly. Low severity (agent-instruction doc only).
| printf '%s %s changed' "$branch" "$changed" | ||
| fi | ||
|
|
||
| if [ -n "$ahead" ] || [ -n "$behind" ]; then |
There was a problem hiding this comment.
[low] In git_status_summary, when git status --porcelain=v2 --branch emits a branch.ab line (upstream configured), ahead/behind are set to numeric strings — including 0 for a branch exactly in sync:
if [ -n "$ahead" ] || [ -n "$behind" ]; then
printf ' (%s ahead, %s behind)' "${ahead:-0}" "${behind:-0}"
fiSince ahead is the non-empty string 0 in that case, [ -n "$ahead" ] is always true and the dashboard appends a noisy (0 ahead, 0 behind) suffix for every clean, up-to-date tracking branch. The guard clearly intends to suppress the suffix when there is nothing to report. Gate on the numeric values:
| if [ -n "$ahead" ] || [ -n "$behind" ]; then | |
| if [ "${ahead:-0}" -gt 0 ] || [ "${behind:-0}" -gt 0 ]; then | |
| printf ' (%s ahead, %s behind)' "${ahead:-0}" "${behind:-0}" | |
| fi |
Cosmetic only (low severity), but it makes the dashboard noisier than intended.
| @@ -0,0 +1,72 @@ | |||
| # Git Branching Workflow Extension Configuration | |||
| # Copied to .specify/extensions/git/git-config.yml on install | |||
There was a problem hiding this comment.
[low] The extension's install manifest declares the config target as git-config.yml generated from the template config-template.yml (extension.yml line 37-38: name: "git-config.yml", template: "config-template.yml"). That makes config-template.yml the single source-of-truth template. The git-config.yml committed alongside it at the extension root is a redundant second copy that nothing references as a source — runtime scripts read the installed copy at $REPO_ROOT/.specify/extensions/git/git-config.yml (e.g. scripts/bash/create-new-feature.sh:262, scripts/bash/initialize-repo.sh:29), never this template-dir file.
Two concrete issues in this newly added file:
- Misleading header (line 2):
# Copied to .specify/extensions/git/git-config.yml on install— this file isgit-config.yml, so the comment claims it is copied to itself. The templateconfig-template.ymlis the file that is copied; this one is not the install source. - Divergent default (line 15):
worktree_root: ../example-repo-worktrees, whereas the canonicalconfig-template.yml:15andREADME.md:63both use../my-repo-worktrees. The two committed defaults disagree in the same commit that adds them.
Runtime impact is limited (scripts also compute a dynamic ../<repo-basename>-worktrees fallback and never read this template-dir copy), hence low severity — but it is a real consistency/maintenance wart that invites a future install/sync bug if the wrong file is ever propagated.
Fix: prefer deleting this redundant git-config.yml so config-template.yml is the only template. If it must stay, align worktree_root to ../my-repo-worktrees and correct the self-referential header.
| # Copied to .specify/extensions/git/git-config.yml on install | |
| # Git Branching Workflow Extension Configuration | |
| # Generated from config-template.yml on install; edit per-repo as needed |
| cmake -S . -B "$BUILD_DIR" -G Ninja \ | ||
| -DCMAKE_BUILD_TYPE=Debug \ | ||
| -DCMAKE_C_FLAGS="${CMAKE_C_FLAGS:-} --coverage" \ | ||
| -DCMAKE_CXX_FLAGS="${CMAKE_CXX_FLAGS:-} --coverage" \ |
There was a problem hiding this comment.
[low] The expansions ${SONAR_CPP_CMAKE_ARGS:-} (line 47), ${SONAR_CPP_CTEST_ARGS:-} (line 50), and ${SONAR_CPP_GCOVR_ARGS:-} (line 56) are intentionally unquoted to permit multiple tokens. Under set -euo pipefail the :- guards avoid unset-variable failures, so simple values work, but unquoted expansion is subject to pathname expansion (globbing) and cannot carry any argument containing spaces. For example SONAR_CPP_CMAKE_ARGS='-DCMAKE_TOOLCHAIN_FILE=/path with space/x.cmake' would split incorrectly, and a value containing * could glob against cwd.
The sibling goBench/files/sonarcloud-go-coverage (lines 40-41) handles the equivalent case correctly:
IFS=' ' read -r -a extra_args <<< "${SONAR_GO_TEST_ARGS:-}"
... "${extra_args[@]}"For parity and safety, parse these into arrays the same way and expand with "${array[@]}", or at minimum document that values must be single, glob-free, space-free tokens.
Severity is low: these are optional advanced knobs and the :- guards prevent set -u errors; impact only surfaces with unusual values.
| # Install testing and performance tools | ||
| RUN go install github.com/rakyll/hey@latest | ||
|
|
||
| COPY files/sonarcloud-go-coverage /usr/local/bin/sonarcloud-go-coverage |
There was a problem hiding this comment.
[low] The added COPY files/sonarcloud-go-coverage + RUN chmod +x (lines 97-98) sit in the middle of the go install sequence — after hey, but before goreleaser, yq, and the apt-get protobuf-compiler block.
Docker layer caching is positional: any change to the sonarcloud-go-coverage script invalidates this COPY layer and therefore every layer after it, re-running the downstream go install (goreleaser, yq) and apt-get steps on each edit of the helper script. Those are network-bound downloads rather than source recompiles, but still minutes of needless rebuild for a one-line script change.
Move the COPY to the end of the tool-install section (e.g. just before the /etc/skel/.zshrc alias block), after the expensive install layers, so editing the script only rebuilds the cheap tail. With BuildKit you can also collapse the two instructions:
| COPY files/sonarcloud-go-coverage /usr/local/bin/sonarcloud-go-coverage | |
| COPY --chmod=755 files/sonarcloud-go-coverage /usr/local/bin/sonarcloud-go-coverage |
(placed after the last go install).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1324168e42
ℹ️ 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".
| test_tool "gemini" "command -v gemini" | ||
| test_tool "opencode" "command -v opencode" | ||
| test_tool "yolo alias" "zsh -ic 'alias yolo' | grep -q 'claude --dangerously-skip-permissions --teammate-mode tmux'" | ||
| test_tool "yolo function" "zsh -ic 'type yolo' | grep -q 'tmux new-session' && zsh -ic 'type yolo' | grep -q -- '--teammate-mode tmux'" |
There was a problem hiding this comment.
In the devcontainer tests this runs under zsh, but type in zsh is equivalent to whence -v (zsh docs/answers describe it as printing only that a name is a shell function unless type -f/whence -f is used), so it will output something like yolo is a shell function rather than the function body. That means both greps for tmux new-session and --teammate-mode tmux fail even when the new yolo function is installed correctly; use a zsh form that prints the definition, such as type -f yolo or functions yolo.
Useful? React with 👍 / 👎.
| copy_overlay_tree(template_root / "agents" / "skills" / "speckit-specify", repo / ".agents" / "skills" / "speckit-specify", dry_run, force) | ||
| copy_overlay_tree(template_root / "agents" / "skills" / "speckit-git-feature", repo / ".agents" / "skills" / "speckit-git-feature", dry_run, force) | ||
| copy_overlay_tree(template_root / "claude" / "skills" / "speckit-specify", repo / ".claude" / "skills" / "speckit-specify", dry_run, force) | ||
| copy_overlay_tree(template_root / "claude" / "skills" / "speckit-git-feature", repo / ".claude" / "skills" / "speckit-git-feature", dry_run, force) | ||
| copy_overlay_tree(template_root / "codex" / "skills" / "speckit-specify", repo / ".codex" / "skills" / "speckit-specify", dry_run, force) |
There was a problem hiding this comment.
When setup-openspeckit initializes or updates a repo without --force, specify init --ai-skills or an existing Speckit checkout has already created these skill files, and copy_overlay_tree(..., force) keeps any existing target unchanged. In that normal path the worktree-aware speckit-specify/speckit-git-feature instructions are not installed for agents, so /speckit.specify can miss the hook/worktree handoff even though the git extension overlay and extensions.yml are configured.
Useful? React with 👍 / 👎.
| template_root = agent_root / "templates" / "claude" / "commands" / "opsx" | ||
| copy_missing_tree( | ||
| template_root, | ||
| repo / ".claude" / "commands" / "opsx", |
There was a problem hiding this comment.
For a fresh devBench user, the Dockerfile seeds the opsx command templates under ~/.claude/commands/opsx, but this bootstrapper looks under ~/.agents/templates/claude/commands/opsx, which ensure_agent_protocol_files() never creates. As a result copy_missing_tree() silently no-ops and setup-openspeckit does not install the promised repo-local .claude/commands/opsx/* commands unless the user has manually created that separate templates tree.
Useful? React with 👍 / 👎.
| find "$specs" -maxdepth 1 -mindepth 1 -type d -name '[0-9][0-9][0-9]-*' \ | ||
| -printf '%T@ %p\n' 2>/dev/null | sort -nr | awk 'NR == 1 { $1=""; sub(/^ /, ""); print }' |
There was a problem hiding this comment.
The new Speckit skill writes .specify/feature.json and explicitly supports timestamp/custom SPECIFY_FEATURE_DIRECTORY values, but the dashboard sync ignores that state and falls back only to a branch-named directory or NNN-* spec dirs. For timestamp-numbered or custom feature directories, the dashboard never finds the active tasks.md (or can pick an unrelated older sequential feature), so cta stays on the initial/stale dashboard despite an active feature.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c4d587266
ℹ️ 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".
| while IFS='=' read -r _sonar_key _sonar_value || [ -n "$_sonar_key" ]; do | ||
| case "$_sonar_key" in | ||
| ''|\#*) continue ;; | ||
| SONARQUBE_TOKEN|SONARQUBE_ORG|SONAR_TOKEN|SONAR_ORGANIZATION|SONAR_HOST_URL|SONARQUBE_URL) |
There was a problem hiding this comment.
When a user’s ~/.config/sonarqube/sonar.env contains SONARQUBE_CLOUD_URL (the alias already handled by ensure-sonarqube-mcp.sh), this allow-list drops it, so the later fallback sets SONAR_HOST_URL/SONARQUBE_CLI_SERVER to https://sonarcloud.io. In SonarQube Cloud US or other non-default deployments, sonar-env and shell-started scanner workflows therefore target the wrong server even though the user provided the URL; include SONARQUBE_CLOUD_URL here and map it the same way as SONARQUBE_URL.
Useful? React with 👍 / 👎.
| 4) | ||
| echo "php" | ||
| return 0 |
There was a problem hiding this comment.
When this detector is sourced by new-workspace.sh, update-workspace.sh, or delete-workspace.sh in a PHP project, choosing the newly added PHP option returns php, but those callers only register and normalize Frappe/Flutter/.NET workspace scripts (for example scripts/new-workspace.sh:221-225 and :315). The result is an empty script lookup and a “script not found” failure instead of falling back to the installed workspace creators, so only show/return PHP after the workspace routers can handle it.
Useful? React with 👍 / 👎.
| if suffix: | ||
| lines[index] = indent + "installed:" | ||
| lines.insert(index + 1, indent + "- git") | ||
| changed = True |
There was a problem hiding this comment.
If an existing .specify/extensions.yml uses a valid inline installed list such as installed: [other-extension], this branch replaces the line with installed: and inserts only - git, dropping the previously installed extension names. Running speckit-worktree-enable in that repo silently unregisters unrelated Spec Kit extensions while adding the worktree hook; expand the existing inline entries instead of discarding the suffix.
Useful? React with 👍 / 👎.
| set_top_level_yml_values( | ||
| repo / ".specify" / "extensions" / "git" / "git-config.yml", | ||
| { | ||
| "branch_numbering": "sequential", | ||
| "checkout_mode": "worktree", | ||
| "base_branch": resolved_base_branch, | ||
| "worktree_root": resolved_worktree_root, | ||
| }, |
There was a problem hiding this comment.
When setup-openspeckit is rerun on a repository that already customized .specify/extensions/git/git-config.yml, this unconditional value set rewrites branch_numbering, base_branch, and worktree_root back to the script defaults unless the user passes matching flags. That makes a routine refresh of repo agent pointers or Speckit overlays silently switch teams from timestamp/custom worktree settings back to sequential branches under ../<repo>-worktrees; only force these defaults when creating the config or when the corresponding option was explicitly provided.
Useful? React with 👍 / 👎.
|
@codex review |
| install -m 0644 "$source_file" "$target_file" | ||
| echo "Installed Claude workflow: $target_file" | ||
| installed=$((installed + 1)) | ||
| done < <(find "$SOURCE_DIR" -maxdepth 1 -type f -name '*.js' -print0 | sort -z) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9d124c9c8
ℹ️ 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".
| payload={"event":"COMMENT","body":"\\n".join(lines),"comments":comments} | ||
| json.dump(payload, open('/tmp/review_payload.json','w')) | ||
| print("MAPPED",len(comments),"UNMAPPED",len(unmapped)) | ||
| PY |
There was a problem hiding this comment.
In PR mode with confirmed findings, this workflow asks the posting agent to run this shell snippet exactly; the closing PY delimiter is indented with spaces, so Bash will not recognize it for the earlier <<'PY' heredoc (unless using <<- with tabs). The generated Python script then never terminates correctly, so automatic PR review posting stalls or fails. Move the heredoc delimiter to column 1 or avoid a heredoc here.
Useful? React with 👍 / 👎.
| if suffix: | ||
| lines[index] = indent + "installed:" | ||
| lines.insert(index + 1, indent + "- git") |
There was a problem hiding this comment.
When an existing .specify/extensions.yml uses an inline list such as installed: [foo, bar], this branch replaces the whole value with only installed:\n- git, dropping every previously installed extension before adding the worktree hook. That silently disables other Spec Kit extensions whenever worktree support is enabled on such a repo; append git to the parsed inline list instead of replacing it.
Useful? React with 👍 / 👎.
Summary
Validation
Summary by Sourcery
Align shared base and devBench tooling for AI/spec CLIs, SonarQube integrations, and spec-kit workflows while adding PHP bench support and improving workspace detection and tests.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores: