Skip to content

fix(ui): reject reserved blank names in builder preview - #349

Merged
kubestellar-prow[bot] merged 1 commit into
hivecommons:mainfrom
thatguywob:fix/ui-reserved-blanks
Oct 26, 2026
Merged

kubestellar-prow[bot] merged 1 commit into
hivecommons:mainfrom
thatguywob:fix/ui-reserved-blanks

Conversation

@thatguywob

Copy link
Copy Markdown
Contributor

Summary

The builder now validates template blanks with the CLI's shared reserved-name rule. Names such as json and help show the canonical error in the preview and reset the generated command until the blank is renamed. Serve the shared flags module to the browser and include the rebuilt distribution and changelog fragment.

Fixes #347

Validation

  • npm run lint
  • npm run build
  • npm test (full suite and coverage gates pass)
  • Regression cases cover all five reserved names, defaults, empty values, and recovery after renaming; HTTP coverage verifies /flags.js is served as JavaScript.

Checklist

  • Lint, build, and tests pass locally
  • Rebuilt dist/ is committed
  • User-visible changelog fragment included
  • Behavior changes include tests
  • Commit signed off with DCO

— hive: backend=codex model=gpt-6.1-sol


🐝 Hive Agent: contributor | SHA: aeb9be9

Hive-Run: hivecommons#347
Hive-Plan: reserved-blank-validation
Hive-Spec: issue-347#expected-preview-error
Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
@kubestellar-prow kubestellar-prow Bot added the dco-signoff: yes Indicates the PR's author has signed the DCO. label Oct 26, 2026
@github-actions

Copy link
Copy Markdown

Welcome to the Hive Commons community!

Thanks for opening your first PR here! We're excited to have you contributing.

Before merge, please ensure:

  • DCO Sign-off — All commits signed with git commit -s (DCO)
  • PR Title — Starts with an emoji: ✨ feature | 🐛 bug fix | 📖 docs | 🌱 infra/tests | ⚠️ breaking

A maintainer will review your PR soon. Hope you have a great time here!

@kubestellar-prow kubestellar-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 26, 2026
@hivecommons-hive hivecommons-hive Bot added author/clanker A contributor's own hive authored this PR under their login dco All commits are DCO signed-off role/contributor Author holds the contributor tier in this hive's registry; a human must lgtm/approve stage/050-review-pending Review swarm requested; awaiting a verdict for the current head labels Oct 26, 2026
@hivecommons-hive

Copy link
Copy Markdown
Contributor

Overlap note: #348 (hive, opened 05:21Z) fixes the same issue #347 and touches the same files (src/ui.html, src/ui.ts, src/ui-builder.test.ts, src/ui.test.ts, dist/). Whichever lands first should close the other; the second will need a rebase onto main. This PR's build-and-test matrix has not run yet (fork workflows await maintainer approval).


🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the diff against main and the test harnesses it touches — this looks correct to me, and it is the one of the two #347 PRs that should land.

  • Production change is the minimal one: src/ui.html:662 imports assertNoReservedVars from the shared ./flags.js, src/ui.html:861 calls assertNoReservedVars(parseVars(text)) inside update()'s existing try block (so a reserved blank falls through to the same preview-empty error path as a parse error), and src/ui.ts:82 adds /flags.js to the static-route table.
  • Because the builder script is compiled in tests via compileInlineScript(...) with an explicit parameter list, every harness has to pass the new name. This PR updates all four — src/ui-bootstrap.test.ts:270-272, src/ui-builder.test.ts:95-98, src/ui-command-quoting.test.ts:53-55, src/ui-interactions.test.ts:256-268. #348 (same fix, hive-authored) updates only ui-builder and is failing CI with assertNoReservedVars is not defined in the other three; I've flagged it there as a duplicate of this one.
  • Tests cover all five reserved names with and without a default, empty values, and recovery after renaming (src/ui-builder.test.ts:238-261).

Not verified: CI has not run on this fork head yet, so the committed dist/ sync is unchecked by me — the dist-sync job will settle that.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot added stage/060-reviewed Review swarm verdict approve; a gate is still pending (CI running, soak not started) and removed stage/050-review-pending Review swarm requested; awaiting a verdict for the current head labels Oct 26, 2026

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct to me. Read the diff against main and the callers; verified locally.

  • src/ui.html:862 calls assertNoReservedVars(parseVars(text)) inside update()'s existing try, which is the same call the CLI makes at src/cli.ts:140 and src/commands.ts:114, so the preview and the CLI now reject the same names with the same message. assertNoReservedVars (src/flags.ts:19) only reads .name, which parseVars' TemplateVar[] provides.
  • src/ui.ts:82 adds /flags.js to STATIC_JS_ROUTES; src/ui.test.ts:288-291 asserts it is served with a text/javascript content-type.
  • All four compileInlineScript harnesses (ui-bootstrap.test.ts:270, ui-builder.test.ts:95, ui-command-quoting.test.ts:52, ui-interactions.test.ts:256) inject the new identifier. This is the part #348 — which makes the identical production change — missed, and why that PR fails 23 tests with assertNoReservedVars is not defined.
  • Built pr349 from a fresh npm ci: npm test → 486 pass, 0 fail, coverage gates pass; git status --porcelain -- dist/ is clean, so the dist-sync gate will pass.

Note for the maintainer: #348 is a duplicate of this one (same fix for #347); I've recommended closing it in favour of this PR.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot added stage/090-ready-for-human-merge Hive would merge this but trust/role blocks it; a maintainer's /lgtm /approve is all that is missing and removed stage/060-reviewed Review swarm verdict approve; a gate is still pending (CI running, soak not started) labels Oct 26, 2026
@hivecommons-hive

Copy link
Copy Markdown
Contributor

Ready for a human merge: verdict approved @aeb9be9, 3/3 checks green, DCO yes, soak 30m0s elapsed. Blocked only by role/contributor (no verified hive claim or registry tier).

The hive never approves on your behalf: /lgtm /approve (or merge) if you accept it.

@hivecommons-hive

Copy link
Copy Markdown
Contributor

CI triage: the CI / build-and-test run for this head is in action_required — GitHub is holding the workflow for a first-time fork contributor until a maintainer clicks Approve and run on https://github.com/hivecommons/promptargs/actions/runs/40274166469. Until then the required test check cannot report and the PR stays BLOCKED regardless of review state. No fault in the diff; nothing for automation to retry.


🐝 Hive Agent: quality | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

— hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot added stage/060-reviewed Review swarm verdict approve; a gate is still pending (CI running, soak not started) stage/stalled In its current stage longer than expected and removed stage/090-ready-for-human-merge Hive would merge this but trust/role blocks it; a maintainer's /lgtm /approve is all that is missing labels Oct 26, 2026

@clubanderson clubanderson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hive verdict approved, soak done; releasing. Thanks @thatguywob.

@clubanderson

Copy link
Copy Markdown
Member

/lgtm
/approve

@kubestellar-prow kubestellar-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2026
@kubestellar-prow

Copy link
Copy Markdown

LGTM label has been added.

DetailsGit tree hash: 6aa2c1ae024a98a264232b3f4ed6bd928c9bca87

1 similar comment
@kubestellar-prow

Copy link
Copy Markdown

LGTM label has been added.

DetailsGit tree hash: 6aa2c1ae024a98a264232b3f4ed6bd928c9bca87

@kubestellar-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clubanderson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubestellar-prow kubestellar-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2026
@hivecommons-hive hivecommons-hive Bot removed the stage/stalled In its current stage longer than expected label Oct 26, 2026
@kubestellar-prow
kubestellar-prow Bot merged commit 453f194 into hivecommons:main Oct 26, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. author/clanker A contributor's own hive authored this PR under their login dco All commits are DCO signed-off dco-signoff: yes Indicates the PR's author has signed the DCO. lgtm Indicates that a PR is ready to be merged. role/contributor Author holds the contributor tier in this hive's registry; a human must lgtm/approve size/L Denotes a PR that changes 100-499 lines, ignoring generated files. stage/060-reviewed Review swarm verdict approve; a gate is still pending (CI running, soak not started)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[scanner] ui: builder emits a command the CLI rejects when a blank uses a reserved switch name (json/cross/status/no-interactive/help)

2 participants