Skip to content

fix(cli): resolve technical debt (D-0947, D-1416, D-0916, D-0930, D-0918) - #1004

Open
acamarata wants to merge 12 commits into
mainfrom
dw9-cli-18
Open

acamarata wants to merge 12 commits into
mainfrom
dw9-cli-18

Conversation

@acamarata

@acamarata acamarata commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Fix six verified debt rows from fresh origin/main (68dc16a0), with one commit per row.

  • D-0947: Serialize per-user crontab installation/removal, including cron self-removal, with the same file lock. Retire timers only when the binary is absent (test -e); update the guard contract and golden fixtures. Serialize entitlement publication and use transactional replacement when host inspection proves the file is unmounted. Preserve mounted inodes, or preserve them conservatively if inspection is unavailable. Supply host inspection from the CLI entry point through the Docker funnel without increasing its importer budget.
  • D-0844: Already fixed on origin/main (openArchive and extractTarGzFrom exist); this PR only pins the contract with a non-vacuous checksum assertion in archive_snapshot_test.go.
  • D-1416: Remove the unconditional published-file cache hit. Under the existing install operation lock, resolve the current catalogue entry and recheck a receipt binding its version, signing metadata, binary name, and published bytes. Re-run signature trust policy with this CLI's keys. Missing, outdated, altered, or manual installs use the verified seam; a second first-run caller reuses the completed verified install. Adjacent development checkouts retain their development build route.
  • D-0916: Read bundled manifest next_steps in the normal lifecycle and template-info handlers, including JSON. Remove both RunE wrappers. Refresh affected JSON schemas, OpenAPI, MCP goldens, and generated TypeScript types.
  • D-0930: Do not stage certificate bytes when the certificate directory has a scheduled regeneration effect. The plan-bound apply installs the newly generated pair as a runtime effect.
  • D-0918: Register AUTH_JWT_CUSTOM_CLAIMS and recognize the full AUTH_ / HASURA_AUTH_ namespaces already accepted by compose passthrough. Remove the multi-app template's ENV_ALLOWLIST workaround and update the config-list golden.

All six problems reproduced against the original source. Clean-main regression comparisons fail for concurrent cron updates, non-executable-but-present binaries, concurrent entitlement publication, archive-path replacement, unreceipted CI binaries, manifest steps, expired certificate staging, and auth warnings; corresponding checks pass with these changes. Archive comparison uses the original path-based verification/extraction entry points on the baseline and the snapshot entry points after the fix.

Validation:

  • PASS: go test -mod=vendor ./internal/repoqa ./internal/maintenance ./internal/license/entitlement ./internal/config ./internal/templates/clone ./internal/build ./internal/plugin ./internal/docker -count=1 -timeout 3m
  • PASS: go test -mod=vendor ./cmd/commands ./internal/mcpgen -count=1 -timeout 5m (whole command and MCP packages).
  • PASS: focused go test -race -mod=vendor checks for concurrent cron installation, the presence guard, serialized entitlement writes, and atomic unmounted refresh.
  • PASS: go vet -mod=vendor for all touched Go implementation packages; go build -mod=vendor -o <scratch>/nself ./cmd/nself; bash scripts/ci/gofmt-check.sh and go test -mod=vendor ./internal/repoqa -run TestGofmtClean (both re-run at head 3697c74b after gofmt -w on cmd/commands/relocate.go, plugin_autoinstall.go and ci_bootstrap_test.go; the earlier head failed both); git diff --check.
  • PASS: CI-pinned golangci-lint v2.13.2 with GOTOOLCHAIN=go1.26.10, run --timeout=5m over all touched Go implementation packages (0 issues). The default worker Go 1.27.3 is incompatible with this linter's export-data reader, also reproduced on clean main; using the repository-pinned toolchain resolves that tool mismatch.
  • PASS: schema/OpenAPI/TypeScript generators and repository drift checks; MCP tool golden regenerated with go test -mod=vendor ./internal/mcpgen -run '^TestToolListGolden$' -update.
  • PASS: generated TypeScript client checked in an isolated copy with workflow-pinned pnpm 9: npx --yes pnpm@9 install --frozen-lockfile, npx --yes pnpm@9 run typecheck, and TZ=America/New_York npx --yes pnpm@9 test -- --testPathIgnorePatterns live (24 tests).
  • Additional v1.5 checks: all affected library packages pass except three existing remote-config tests rejecting this worker's exported NO_COLOR=1: TestLoadRemoteDeployDropsEnvLocal, TestRemoteLoadLeavesNoState, and TestLoadValidateConfig_RemoteDeployDropsEnvLocal. The same three failures reproduce on clean origin/main with NSELF_V15=1. Focused v1.5 command checks (TestCIBootstrap|TestAutoInstall|TestTemplate|TestPilotJSONConfig|TestMCPGeneratedStdio) pass.

No Docker daemon, production writes, release/version changes, or HQ edits. No repository instruction files or active hooks were present.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Rebased onto origin/main to resolve merge conflicts in internal/plugin/{cli_collision_test,download,installer_locked,installer_signature,security}.go. Kept origin/main's openArchive/verifyChecksumSum/extractTarGzFrom approach (D-1326) which already satisfies D-0844's single-snapshot guarantee; rewrote archive_snapshot_test.go to exercise that contract via the same helpers. All plugin tests pass with -mod=vendor.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Rebased onto origin/main and regenerated .github/facts.json (stale after main moved). TestFactsCurrent/TestReadmeFactsCurrent now pass locally; pushed b46489a to dw9-cli-18.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Fixed both review findings:

  1. Cron self-removal lock dir (359ceba): The 9>"$HOME/.nself/entitlement-cron.lock" redirect was evaluated before the subshell ran, so mkdir -p inside the subshell did not help. Restructured to { mkdir -p "$HOME/.nself" && (...flock...) 9>...; } so the directory exists before the redirect is set up. Added TestCronSelfDeleteCreatesLockDir that runs the real cron line through sh with no ~/.nself and proves the line is removed instead of failing silently. Updated the crontab golden.

  2. D-0844 commit message (919f03a): Reworded from fix(plugin): verify and extract one archive snapshot (D-0844) to test(plugin): pin archive snapshot contract from D-1326 (D-0844), clarifying this is a test-only pin of the fix already landed in 4628857 (D-1326). The debt row resolution should point at that earlier commit.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Fixed both review findings in b0828d2:

  1. Package-size ceiling: moved autoInstallCached, autoInstallRecord, autoInstallBinaryName, and readInstalledVersion from cmd/commands/plugin_autoinstall_cache.go into internal/plugin/autoinstall/receipt.go. cmd/commands is now under its 65362 ceiling; updated the repoqa allowlist for the pre-existing internal/plugin growth from archive_snapshot_test.go.

  2. Offline template info scope: limited the bundled-manifest shortcut in runTemplateInfo to multi-app only, restoring the behavior that TestTemplateInfoBundledOffline expects. Other slugs still go through the registry.

Both TestPackageSizeBudget and TestTemplateInfoBundledOffline pass.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Review fix: ran gofmt -w on cmd/commands/relocate.go, plugin_autoinstall.go and ci_bootstrap_test.go (import ordering/indentation) and amended b0828d2 (now 3697c74, force-with-lease). Before: TestGofmtClean and scripts/ci/gofmt-check.sh failed on those 3 files. After: both pass; go build, go vet ./cmd/commands and the focused cmd/commands tests (TestCIBootstrap|TestAutoInstall|Relocate) pass. PR body validation line corrected to match.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Rebased onto origin/main (92322be, #1009) to clear the merge conflict. One conflict, in internal/repoqa/testdata/package-size-allowlist.txt: both sides edited the internal/plugin ceiling. Kept this branch's 10092, which covers archive_snapshot_test.go; TestPackageSizeBudget passes. Checks after the rebase: go build ./..., go vet, gofmt clean, internal/plugin/..., maintenance, config, build, pluginname, repoqa and tools/facts all pass. The 162 cmd/commands tests for autoinstall, relocate, ci-bootstrap, template info, gofmt, package size, cron and golden pass. The full cmd/commands run hit the 600s timeout, so I ran those tests by name instead. Head is now bb814bf.

This commit adds only a regression test for openArchive, verifyChecksumSum
and extractTarGzFrom, which were already fixed on origin/main in commit
4628857 (D-1326). The debt row resolution should point at that earlier
commit; this test pins the contract so future refactors cannot regress it.
Move autoInstallCached, autoInstallRecord, autoInstallBinaryName, and
readInstalledVersion into internal/plugin/autoinstall so cmd/commands
stays under its package-size ceiling. Limit the offline template info
path in runTemplateInfo to multi-app only, restoring the pre-PR scope
that TestTemplateInfoBundledOffline expects. Update the repoqa allowlist
for the pre-existing internal/plugin growth from archive_snapshot_test.go.
@acamarata

Copy link
Copy Markdown
Collaborator Author

Rebased onto origin/main (86627b8) to clear the merge conflict. One conflict, in .github/facts.json (generated). Took main's copy mid-rebase and regenerated it with tools/facts, so the last commit carries the refreshed file. Checks after the rebase: go build ./..., go vet, gofmt clean. Tests pass for internal/plugin/..., maintenance, config, build, pluginname, repoqa and tools/facts (2855 tests). The 156 targeted cmd/commands tests (autoinstall, relocate, ci-bootstrap, template info, gofmt, package size, cron, golden, facts) pass. The full cmd/commands run was not repeated because it exceeds the timeout.

@acamarata acamarata changed the title fix(cli): resolve technical debt (D-0947, D-0844, D-1416, D-0916, D-0930, D-0918) fix(cli): resolve technical debt (D-0947, D-1416, D-0916, D-0930, D-0918) Oct 28, 2026
- archive_snapshot_test: verify against the real expected SHA-256 of the
  original archive bytes instead of comparing the sum with itself.
- entitlement_timer: guard cron self-removal with 'command -v flock' so
  hosts without flock still drop the stale line instead of failing
  silently and running forever.
- Update golden fixture and flock test stubs to match the guarded shell
  shape.
@acamarata

Copy link
Copy Markdown
Collaborator Author

Review fixes pushed in 361592f:

  • D-0844: Already fixed on origin/main (openArchive/extractTarGzFrom exist). Updated archive_snapshot_test.go to verify against the real expected SHA-256 of the original archive bytes instead of the vacuous sum == sum check. PR title/body updated to reflect this.
  • Cron flock guard: entitlement_timer.go now guards self-removal with command -v flock; hosts without flock fall back to unlocked removal so the stale line is still dropped. Golden fixture and macOS flock test stubs updated to match.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Fixed gofmt formatting in internal/maintenance/entitlement_timer.go (renderCron). scripts/ci/gofmt-check.sh and internal/repoqa TestGofmtClean both pass. Pushed as 6dc5a3d.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Removed the unused isRegularFile from cmd/commands/ci_bootstrap.go (its only callers moved to internal/plugin/autoinstall, which has its own copy). Build, vet and the ci/bootstrap tests pass. golangci-lint isn't installed locally, so the lint run itself is unverified.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant