Repository navigation
fix(shell): allow the test environment command - #1062
Conversation
The command inventory test rejects the Go-built `testenv` helper because its name is missing from the published allow list. Assisted-by: codex:gpt-6.1-sol Signed-off-by: Tony Burns <tony@tonyburns.net>
tbhb-agent
added
change:fix
tbhb-agent
added
change:fix
![tbhb-dev-reviews[bot]](https://avatars.githubusercontent.com/u/288164213?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed head: 5cade51
Review: CHANGES REQUESTED
Loosens: yes. .devtools/shell-allowlist.toml:85 newly permits the Go-built testenv helper by name. The added line matches none of .agents/agent-skills.conf:6 loosening_markers; that absence does not negate the semantic relaxation. No t.Skip, nolint, weakened assertion, analyzer modification, or other gate change.
Owner-tier: no. Changed paths: .devtools/shell-allowlist.toml:85 — owner-tier no; default CODEOWNERS owner is @tbhb. No protected path from the requested owner-tier inventory changed.
No linked issue, issue comments, or Refinement verdict exists for this repair. The following are review-local identifiers for the requested acceptance criteria, not pre-existing issue AC IDs.
| AC | Met on reviewed head | Evidence |
|---|---|---|
| AC1: smallest documented repair | Yes | Exactly one added command entry, .devtools/shell-allowlist.toml:85; command entries are the documented mechanism, docs/shell-allowlist.md:15; helper exists at cmd/testenv/main.go:13. |
| AC2: preserve regression assertion and analyzer | Yes | internal/shell/shell_test.go:81-90 still enumerates every command and asserts no findings; neither test nor analyzer changes. |
| AC3: no suppression or unrelated change | Yes | Entire diff is .devtools/shell-allowlist.toml:85; zero added suppression markers. |
| AC4: description layout and failing/passing evidence | Yes | Required Summary, Why, Verification, Risk and Related sections are present. Verification records go test ./internal/shell -run '^TestEveryCmdDirectoryIsAllowed$' -count=1 failing on base 2e5d8d7 and passing after the fix, plus passing mise run test-go. |
| AC5: Verification cites every existing issue AC ID | Yes, vacuous | No linked issue or existing numbered AC IDs. Verification has no AC-ID citations; review-local IDs above do not create an issue requirement retroactively. |
Size: scc 4.1.0 reports 1 added TOML code line, 0 removed lines and 0 prose lines, below 800 code / 2000 prose. Whole changed file: 95 code, 5 comment, 3 blank lines. Per-file private-string scan: zero hits for the exact-head contents of the sole changed file. All 23 check contexts were read once and were COMPLETED / SUCCESS. MERGEABLE; mergeStateStatus BLOCKED; not a draft. Mechanical gates were not rerun.
Findings:
- P2 —
.devtools/shell-allowlist.toml:85: This changes the published payload, but Verification does not provide the required consumer-impact disposition (devtools-3), and does not explain documentation impact (D6). Add the consumer adoption explanation, or "No consumer impact" with its reason, and "No doc impact" with its reason if existing adoption documentation remains sufficient. The one-line implementation itself is correct and minimal. No unnecessary guard, option, abstraction, or layer was added.
Definition of done (Defect):
- D1: Before/after commands are documented, but verification on the eventual merged head is pending; no issue AC IDs exist.
- D2: This independent exact-head review requests the Verification correction above; resolution and approval remain pending.
- D3: All 23 contexts succeeded; no exact-head owner approval exists for the semantic gate relaxation.
- D4: Gitleaks succeeded; the exact-head changed file passed the private-string leak scan with zero hits; review text checked for credentials and private data and submitted through the posting leak check.
- D5: The sole branch commit has GitHub verified=true, reason=valid; eventual squash signature remains pending.
- D6: Verification lacks the required documentation-impact explanation; see P2.
- D7: Branch commit has Assisted-by and Signed-off-by, and provenance:agent-assisted label exists; eventual squash Assisted-by remains pending.
- D8: 1 code line / 0 prose lines; within limits.
- D9: No deferred implementation or follow-up work; Related says None. The review correction is required before approval.
- D10: No linked issue; merge and final status evidence are not yet available.
- defect-1: Existing
TestEveryCmdDirectoryIsAllowedregression is unchanged; description records failure before and success after; exact-head Go test checks succeeded. - devtools-1: CI lint, test, and coverage contexts succeeded. Description names
mise run test-go, not all ofmise run check,mise run cover-go-check, andmise run cover-py-check; those exact command results are not independently established here. - devtools-2: Not applicable; no skill source or generated skill changed.
- devtools-3: Published allow-list entry changes; Verification needs the consumer-impact disposition identified in P2.
Round: 1 of 3
There was a problem hiding this comment.
Reviewed head: 5cade51
Review: CHANGES REQUESTED
Loosens: yes, semantically. .devtools/shell-allowlist.toml:85 makes a previously rejected command name pass analysis. The added line matches none of .agents/agent-skills.conf:6 loosening_markers. No t.Skip, nolint, weakened assertion, analyzer change, or additional gate change.
Owner-tier: no. Sole changed path: .devtools/shell-allowlist.toml:85 — owner-tier no; default CODEOWNERS owner @tbhb. None of the specified protected paths changed.
The earlier Verification finding is resolved: the description now explains both documentation and consumer impact, supported by docs/shell-allowlist.md:3-15. No linked issue, issue comments, or Refinement verdict exists. AC identifiers below are local to this review; there are no existing issue AC IDs for Verification to cite.
| AC | Met on this head | Evidence |
|---|---|---|
| AC1: smallest repair using the documented mechanism | Yes | One added command entry at .devtools/shell-allowlist.toml:85; mechanism at docs/shell-allowlist.md:15; helper at cmd/testenv/main.go:13. |
| AC2: preserve the test and analyzer without suppression | Yes | internal/shell/shell_test.go:81-90 still enumerates every command directory and asserts no findings; the entire diff is the entry at .devtools/shell-allowlist.toml:85. |
| AC3: description layout and before/after evidence | Yes | Summary, Why, Verification, Risk and Related are present. Verification names the targeted go test ./internal/shell -run '^TestEveryCmdDirectoryIsAllowed$' -count=1, failing on base 2e5d8d7 and passing after the entry, plus passing mise run test-go. |
| AC4: Verification cites every existing AC ID | Yes, vacuous | No linked issue or existing numbered AC IDs. |
| AC5: accurate loosening disposition | No | The Risk section says “Loosens: no”, although .devtools/shell-allowlist.toml:85 expands the commands analysis accepts. Documentation of the mechanism does not change that effect. |
Size: scc 4.1.0 classifies the addition as 1 TOML code line; 0 deletions, 0 prose lines, within 800 code / 2000 prose. Whole changed file: 95 code, 5 comment, 3 blank lines. Exact-head file scan with the private-string lists: zero hits. All 23 check contexts were read once and are COMPLETED / SUCCESS. MERGEABLE, mergeStateStatus BLOCKED, not a draft. Mechanical gates were not rerun.
Findings:
- P2 —
.devtools/shell-allowlist.toml:85(description Risk): Correct “Loosens: no” to acknowledge the semantic allow-list expansion. Retain the accurate statement that no configured loosening marker matches. The documented mechanism explains why this is the right fix; it does not mean the gate accepts the same inputs as before. No exact-head owner approval is present in the reviewed approvals.
The implementation is minimal and correct. No unnecessary guard, option, abstraction, or layer was added.
Definition of done (Defect):
- D1: Before/after evidence is documented; verification on the eventual merged head remains pending. No issue AC IDs exist.
- D2: Earlier Verification finding resolved; the Risk correction above remains unmet, so exact-head approval is withheld.
- D3: All 23 contexts passed; exact-head owner approval for the semantic relaxation remains unmet.
- D4: Gitleaks passed; the sole exact-head changed file scanned clean with private-string lists; review text inspected and submitted through the posting leak check.
- D5: Branch commit verified=true, reason=valid; eventual squash signature pending.
- D6: Verification says “No doc impact” and explains the existing mechanism documentation at
docs/shell-allowlist.md:15. - D7: Branch commit has Assisted-by and Signed-off-by; provenance:agent-assisted label present; squash trailer pending.
- D8: 1 code / 0 prose lines, within limits.
- D9: No deferred implementation work or follow-ups; Related says None. The remaining description correction is required here.
- D10: No linked issue; merge, board completion, and final Status comment are not yet established.
- defect-1: Unchanged regression test, documented failure before and success after, and successful exact-head Go test checks.
- devtools-1: CI lint, test and coverage passed; exact results for all three named commands
mise run check,mise run cover-go-check, andmise run cover-py-checkare not established by the description. - devtools-2: Not applicable; no skill sources changed.
- devtools-3: Verification says “No consumer impact” with its reason;
docs/shell-allowlist.md:3-9supports no consumer configuration changes.
Round: 2 of 3
There was a problem hiding this comment.
Reviewed head: 5cade51
Review: CHANGES REQUESTED
Loosens: yes, semantically. .devtools/shell-allowlist.toml:85 permits a previously rejected Go-built helper. No added line matches .agents/agent-skills.conf:6 loosening_markers. No t.Skip, nolint, weakened assertion, analyzer change, or other gate change.
Owner-tier: no. Changed paths: .devtools/shell-allowlist.toml:85 — owner-tier no. CODEOWNERS assigns @tbhb by default; none of the specified protected paths changed.
The live description still says “Loosens: no” in Risk, contrary to the re-review brief. The earlier Verification finding is fixed: consumer and documentation impact are explained with accurate references to docs/shell-allowlist.md:3-15. No linked issue, issue comments, or Refinement verdict exists. These AC IDs are review-local; Verification has no existing issue AC IDs to cite.
| AC | Met on this exact head | Evidence |
|---|---|---|
| AC1: smallest documented repair | Yes | Sole addition .devtools/shell-allowlist.toml:85; documented mechanism docs/shell-allowlist.md:15; helper cmd/testenv/main.go:13. |
| AC2: unchanged assertion and analyzer, no suppression | Yes | internal/shell/shell_test.go:81-90 enumerates every command directory and asserts no findings. Entire diff is the one allow-list entry. |
| AC3: description layout and failing/passing commands | Yes | Summary, Why, Verification, Risk and Related are present. Verification records the targeted go test ./internal/shell -run '^TestEveryCmdDirectoryIsAllowed$' -count=1 failing on clean base 2e5d8d7 and passing after the entry, plus passing mise run test-go. |
| AC4: Verification cites every existing issue AC ID | Yes, vacuous | No linked issue or existing numbered issue AC IDs. |
| AC5: accurate loosening statement | No | Risk still says “Loosens: no”; .devtools/shell-allowlist.toml:85 expands accepted command names. The no-marker-match and documented-mechanism statements are both accurate. |
Size: scc 4.1.0 reports 95 TOML code lines on the head versus 94 on the base: 1 added code line, 0 deleted, 0 prose, below 800 code / 2000 prose. Sole exact-head changed file: zero private-string scan hits. All 23 check contexts were read once and were COMPLETED / SUCCESS. MERGEABLE; mergeStateStatus BLOCKED; not a draft. Mechanical gates were not rerun.
Findings:
- P2 —
.devtools/shell-allowlist.toml:85(description Risk): replace “Loosens: no” with an honest acknowledgment that acceptingtestenvrelaxes the allow list. Retain the accurate no-marker-match statement and documentation reference. This is the unresolved round-2 finding; the code is correct and minimal. - D3 remains unmet: no owner approval of this exact head appears in the reviews. App review does not substitute for that approval.
No guard, option, abstraction, or layer beyond the repair was added.
Definition of done (Defect):
- D1: Before/after verification is documented; no issue AC IDs exist. Verification on the eventual merged head remains pending.
- D2: The exact-head independent review still finds the incorrect Risk statement above; approval is withheld.
- D3: All 23 contexts passed; owner approval of the exact head for the semantic relaxation is absent.
- D4: Gitleaks succeeded; sole exact-head changed file passed the private-string scan with zero hits; review text inspected and submitted through the posting leak check.
- D5: Sole branch commit has GitHub verified=true, reason=valid; eventual squash signature remains pending.
- D6: Verification explains “No doc impact”; existing mechanism is documented at
docs/shell-allowlist.md:15. - D7: Branch commit has Assisted-by and Signed-off-by; provenance:agent-assisted label present. Eventual squash trailer remains pending.
- D8: 1 code / 0 prose lines, within limits.
- D9: No deferred implementation or follow-ups; Related says None. The Risk correction is required here.
- D10: No linked issue; merge, board completion and final Status evidence are not yet available.
- defect-1: Unchanged regression assertion at
internal/shell/shell_test.go:81-90; documented failure before and pass after; successful exact-head Go checks. - devtools-1: CI lint, tests and coverage passed; exact results for
mise run check,mise run cover-go-checkandmise run cover-py-checkare not established by the description. No rerun performed. - devtools-2: Not applicable; no skill sources or generated output changed.
- devtools-3: Verification explains “No consumer impact”;
docs/shell-allowlist.md:3-9confirms no consumer configuration change is needed.
Round: 3 of 3
There was a problem hiding this comment.
Reviewed head: 5cade51
Review: CHANGES REQUESTED
Loosens: yes. .devtools/shell-allowlist.toml:85 permits a previously rejected Go-built command. The addition matches none of .agents/agent-skills.conf:6 loosening_markers; the absence of a marker does not negate that semantic relaxation. No t.Skip, nolint, weakened assertion, analyzer change, or other gate change.
Owner-tier: no. Sole changed path: .devtools/shell-allowlist.toml:85 — owner-tier no. Default owner is @tbhb (.github/CODEOWNERS:6); none of the specified protected paths changes.
The earlier Risk finding is resolved: the description now accurately says “Loosens: yes”, accurately reports no matching markers, and correctly cites the documented mechanism at docs/shell-allowlist.md:15. The earlier consumer-impact and documentation-impact findings remain resolved. However, the current Verification table has dropped the failing baseline result that earlier descriptions recorded; it now records only the targeted command passing and green CI.
No linked issue, issue comments, or Refinement verdict exists. AC IDs below are review-local, not existing issue IDs that the description must cite.
| AC | Met on this exact head | Evidence |
|---|---|---|
| AC1: smallest documented repair | Yes | Entire diff is one entry at .devtools/shell-allowlist.toml:85; mechanism at docs/shell-allowlist.md:15; helper at cmd/testenv/main.go:13. |
| AC2: preserve assertion and analyzer; no suppression | Yes | internal/shell/shell_test.go:81-90 still enumerates every command directory and asserts no findings. Neither test nor analyzer changes. |
| AC3: description layout and failing/passing evidence | No | Summary, Why, Verification, Risk and Related are present. Verification records the targeted command passing on 5cade51, but no failing baseline command/result. See finding below. |
| AC4: Verification cites every existing issue AC ID | Yes, vacuous | No linked issue or existing issue AC IDs. |
| AC5: honest loosening and impact disposition | Yes | Risk acknowledges the acceptance expansion at .devtools/shell-allowlist.toml:85; documentation references docs/shell-allowlist.md:3-15 support the consumer and documentation dispositions. |
Size: scc 4.1.0 reports 95 TOML code lines on the head versus 94 on base 2e5d8d7: 1 added code line, 0 deleted, 0 prose, within 800 code / 2000 prose. The sole exact-head changed file passed the private-string leak scan with zero hits. All 23 check contexts were read once and were COMPLETED / SUCCESS. MERGEABLE; mergeStateStatus BLOCKED; not a draft. Mechanical gates were not rerun.
Findings:
- P2 —
.devtools/shell-allowlist.toml:85(description Verification): restore the before-fix evidence forgo test ./internal/shell -run '^TestEveryCmdDirectoryIsAllowed$' -count=1, identifying the failing base and failure result alongside the existing passing head/result. The present table documents only success, so it does not satisfy the requested failing/passing evidence. Preserve the corrected Risk and impact explanations. The code itself is correct and minimal.
D3 remains unmet: no owner APPROVED review of this exact head is present. App review cannot supply the owner's approval for the relaxation. This is an approval requirement, separate from the P2 description finding.
No unnecessary guard, option, abstraction, or layer was added.
Definition of done (Defect):
- D1: No existing issue AC IDs; before-fix evidence is missing from current Verification. Eventual merged-head verification remains pending.
- D2: Code is correct; independent approval is withheld for the Verification finding above.
- D3: All 23 checks succeeded; exact-head owner approval for the relaxation is absent.
- D4: Gitleaks succeeded; the exact-head changed file passed the private-string scan with zero hits; this post was inspected for secrets and is submitted through the posting leak check.
- D5: Sole branch commit has verified=true, reason=valid; eventual squash signature remains pending.
- D6: Verification explains “No doc impact” using
docs/shell-allowlist.md:15. - D7: Branch commit contains Assisted-by and Signed-off-by; provenance:agent-assisted label exists. Eventual squash trailer remains pending.
- D8: 1 code / 0 prose lines, within limits.
- D9: No deferred implementation or follow-ups; Related says None. The description correction is required here.
- D10: No linked issue to close; final merge and completion evidence remain pending.
- defect-1: The unchanged regression at
internal/shell/shell_test.go:81-90and successful exact-head Go checks support the repair. Current Verification omits the previously documented failing baseline result; restore it without rerunning gates. - devtools-1: CI lint, tests and coverage succeeded. Exact results for
mise run check,mise run cover-go-checkandmise run cover-py-checkare not established by current Verification; no rerun performed. - devtools-2: Not applicable; no skill sources or generated output changed.
- devtools-3: Verification explains “No consumer impact”;
docs/shell-allowlist.md:3-9supports no consumer configuration changes.
Round: 3 of 3
There was a problem hiding this comment.
Reviewed head: 5cade51
Review: CHANGES REQUESTED
Loosens: yes. .devtools/shell-allowlist.toml:85 newly accepts the Go-built testenv command. No added text matches .agents/agent-skills.conf:6 loosening_markers. This is a semantic relaxation despite zero marker matches. No t.Skip, nolint, weakened assertion, analyzer change, or other gate change.
Owner-tier: no. Changed paths: .devtools/shell-allowlist.toml:85 — owner-tier no. Default CODEOWNERS owner is @tbhb (.github/CODEOWNERS:6); none of the specified protected paths changed.
The code and current description are accepted. All earlier description findings are resolved: Verification now records the targeted command failing on base 2e5d8d7 and passing on head 5cade51; consumer and documentation impact are explained; Risk accurately says “Loosens: yes”, reports no marker matches, and cites the documented mechanism. No linked issue, issue comments, or Refinement verdict exists. The AC identifiers below are review-local; there are no existing issue AC IDs for Verification to cite.
| AC | Met on this exact head | Evidence |
|---|---|---|
| AC1: smallest documented repair | Yes | Entire diff is one command entry, .devtools/shell-allowlist.toml:85; documented mechanism docs/shell-allowlist.md:15; Go helper cmd/testenv/main.go:13. |
| AC2: preserve regression assertion and analyzer; no suppression | Yes | Unchanged internal/shell/shell_test.go:81-90 enumerates every command directory and asserts no findings; neither tests nor analyzer changes. |
| AC3: description layout and failing/passing commands | Yes | Required Summary, Why, Verification, Risk and Related sections; Verification records go test ./internal/shell -run '^TestEveryCmdDirectoryIsAllowed$' -count=1 failing on base and passing on this head, plus the green-check read. Regression assertion: internal/shell/shell_test.go:88. |
| AC4: Verification cites every existing issue AC ID | Yes, vacuous | No linked issue or existing issue AC IDs. |
| AC5: accurate relaxation and impact explanations | Yes | Acceptance expansion .devtools/shell-allowlist.toml:85; distribution and configuration documentation docs/shell-allowlist.md:3-9; mechanism docs/shell-allowlist.md:15. |
Size: scc 4.1.0 reports 95 TOML code lines on the head versus 94 on the base: 1 added code line, 0 deleted, 0 prose, within 800 code / 2000 prose. Exact-head contents of the sole changed file passed the private-string leak scan with zero hits. All 23 check contexts were read once and were COMPLETED / SUCCESS. MERGEABLE; mergeStateStatus BLOCKED; not a draft. Mechanical gates were not rerun.
Findings:
- D3 —
.devtools/shell-allowlist.toml:85: exact-head owner approval for this semantic relaxation is absent. The independent App review cannot provide the owner's authorization. Changes are requested for D3 only; no code or description correction is requested. No unnecessary guard, option, abstraction, or layer was added.
Definition of done (Defect):
- D1: Before/after evidence is documented; no issue AC IDs exist. Eventual merged-head verification remains pending.
- D2: All earlier code/description findings are resolved; this exact-head review accepts both but withholds approval for D3 only.
- D3: All 23 contexts succeeded; exact-head owner approval of the relaxation is unmet.
- D4: Gitleaks succeeded; exact-head changed-file scan had zero private-string hits; final review text inspected and scanned before posting.
- D5: Sole branch commit has GitHub verified=true, reason=valid; eventual squash signature remains pending.
- D6: Verification explains “No doc impact” with
docs/shell-allowlist.md:15. - D7: Branch commit has Assisted-by and Signed-off-by; provenance:agent-assisted label present; eventual squash trailer remains pending.
- D8: 1 code / 0 prose lines; within limits.
- D9: No deferred implementation or follow-ups; Related says None.
- D10: No linked issue to close; final merge/completion evidence remains pending.
- defect-1: Description restores failing baseline and passing head for the unchanged regression at
internal/shell/shell_test.go:81-90; exact-head native tests succeeded. - devtools-1: CI lint, native tests and coverage succeeded, but exact
mise run check,mise run cover-go-check, andmise run cover-py-checkresults remain unestablished. In particular, the successful coverage job runscover-go-merge(.github/workflows/ci.yml:248), which does not call the separate threshold gate (.devtools/tasks/go.toml:214-230). This evidence limitation is not a new description finding; no gates were rerun. - devtools-2: Not applicable; no skill sources or generated skills changed.
- devtools-3: Verification explains “No consumer impact”;
docs/shell-allowlist.md:3-9supports no consumer configuration changes.
Round: 3 of 3
|
merge:owner-review: owner approval is the only gate left at head 5cade51 (asserted with --owner-approval-only). tbhb-dev-reviews review state: CHANGES_REQUESTED. |
Superseded: the only finding was the missing owner approval, which the owner gave at this head (APPROVED 2026-11-02T12:49:39Z).
Summary
Permit the repository's Go-built
testenvhelper in shell analysis.Why
The command inventory test requires an allow-list entry for every command
directory. The
testenvhelper needs an entry so that assertion passes.Verification
go test ./internal/shell -run '^TestEveryCmdDirectoryIsAllowed$' -count=1testenvdirectory had no allow-list entry.go test ./internal/shell -run '^TestEveryCmdDirectoryIsAllowed$' -count=1gh pr view 1062 --repo tbhb-dev/devtools --json headRefOid,statusCheckRollupNo consumer impact: the entry covers a repository-owned command directory.
Consumers don't need configuration changes when the allow list arrives, as
documented in docs/shell-allowlist.md:3-9.
No doc impact: the existing documentation describes the command entries
used by the analyzer at docs/shell-allowlist.md:15.
Risk
Loosens: yes.
No configured markers match the added entry, which follows
docs/shell-allowlist.md:15.
An incorrect entry would permit an unintended command name. Revert the
entry to restore the previous list.
Compatibility: none
Related
None