Repository navigation
test(server): promote validated QA regression anchors (static, deploy, shutdown-drain, secrets) - #1791
Merged
Conversation
…est, deploy-symlink, shutdown-drain, secret-store) P-326 (qa516): static `after: 'rest'` ordering (#1574) — REST reachable under fallthrough:false SPA config, auth denials not swallowed by static catch-all, and the misconfiguration warning fires when `after: 'rest'` is omitted. P-328 (qa517): hdb_secret plaintext-leak probe (#715) — `set_secret` (create + rotate) is never surfaced via `read_audit_log` (blocked 403) or generic system-table reads (enc:v1: envelope only). P-329 (qa518): deploy past dangling symlink (#1718) — `package_component` + `deploy_component` complete a full archive and deploy for a source project whose dangling symlink precedes real files in directory walk order. P-330 (qa519): shutdown-drain end-to-end (#1621) — in-flight work registered via `ShutdownDrain` survives a real worker restart; permanently-stalled drains are force-killed at the configured ceiling. P-350 (qa551): DROPPED — already covered by integrationTests/components/static-urlpath.test.ts ("static plugin with root urlPath (#1766)" suite, added in a prior PR). All 4 promoted tests pass twice on 3b59214 (v5.2.0-alpha.5). No product code changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces several comprehensive integration tests and fixtures to verify key system behaviors, including preventing plaintext leaks from secrets in audit logs, validating the shutdown-drain mechanism during worker restarts, ensuring correct static file routing order relative to REST endpoints, and verifying packaging/deployment past dangling symlinks. The review feedback suggests improving the robustness of environment variable parsing in the shutdown-drain test fixture by strictly validating numeric inputs instead of casting them directly with Number().
Contributor
|
Reviewed; no blockers found. |
Missed by --write pass on manually-created fixture file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on-numeric input Bare Number() on the env var silently produced NaN/0 for empty, whitespace, non-numeric, or zero values, breaking the intended task delay. Validate with Number.isInteger + positivity before use, matching the existing convention in integrationTests/database/recordCachingWorkers.ts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kriszyp
marked this pull request as ready for review
July 14, 2026 19:02
cb1kenobi
reviewed
Jul 14, 2026
Review nits (cb1kenobi): - static-after-rest misconfig test: replace the fixed sleep(2000) + weak /after: 'rest'/ regex (which also matches the config option name) with a poll for a distinctive fragment of the actual warning text, matching the polling style already used elsewhere in the file. - Replace the dangling "PROBE 3c" comment reference with a direct pointer to the actual ungranted-Bob test. - Drop the findings[]/console.log debug-dump scaffolding in secret-audit-leak.test.ts's after() — exploratory-QA-era leftover with no assertion value. CI failures: - shutdown-drain-e2e.test.ts failed only on Bun, not Node (verified locally both ways). Root cause: restartWorkers() starts the replacement worker immediately after posting SHUTDOWN whenever the platform can't pre-start a SO_REUSEPORT-sharing replacement (canPreStartReplacement is false for Bun/Windows/macOS), assuming the old worker frees its exclusive listeners (e.g. mqtt) well before the replacement binds. The shutdown-drain feature (#1621) can now delay that release for up to the configured drain ceiling (default 10 minutes), breaking the assumption — the CI Bun run showed the replacement worker EADDRINUSE-failing to bind mqtt while the old worker was still mid-drain. This is a real product gap, not test flakiness; filed as harper#1813 and skipped the suite on Bun (matching the existing win32 skip) pending that fix. - deploy-dangling-symlink.test.ts's Windows failure was a separate, unrelated test (not shutdown-drain related, despite appearing in the same CI run): its 30s post-restart readiness deadline (shared convention with deploy-from-source/deploy-from-github) was missed by ~100ms on a Windows runner. Bumped to 45s for this test, which restarts a component with more packaged files than its siblings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…estart race (#1813) CI's Windows 6/6 shard has failed the "deploy_component from that payload deploys the FULL component" test on every run of this PR, even after fix1 bumped the readiness deadline from 30s to 45s. Investigation traced this to the same root cause already filed as harper#1813 for Bun in this same PR: restartWorkers() starts the replacement worker immediately after posting SHUTDOWN whenever the platform can't pre-start a SO_REUSEPORT-sharing replacement (canPreStartReplacement is false for Windows too, not just Bun/macOS), on the assumption the old worker frees its exclusive listeners well before the replacement binds. The shutdown-drain feature (#1621) can delay that release well past this test's restart-readiness deadline. Windows CI evidence (run 29377543554): the replacement worker resolves its full HTTP/mqtt middleware chain in ~1.2s with no bind errors logged, but the readiness poll then observes nothing for the rest of the 45s window before timing out — consistent with #1813's "not fully root-caused" note about downstream effects beyond the logged bind conflict. Added corroborating evidence to that issue. Skip the suite on win32 (matching the existing win32/Bun skip already used by shutdown-drain-e2e.test.ts for the identical root cause) rather than chase a known, tracked product gap inside a QA-anchor-promotion PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Promotes 4 exploratory-QA-validated regression anchors to the tracked integration suite. No product code changes — test-only.
components/static-after-rest.test.tsfallthrough:false+after:'rest'SPA config; auth denials not swallowed by static catch-all; misconfiguration warning fires whenafter:'rest'is omitted. harper#1574.components/secret-audit-leak.test.tsset_secret(create + rotate) never surfaces plaintext viaread_audit_log(blocked 403) or generic system-table reads (enc:v1:envelope only). harper#715.deploy/deploy-dangling-symlink.test.tspackage_component+deploy_componentsucceed past a dangling symlink positioned early in directory walk order; all entries after it land in the archive and deploy. harper#1718.components/shutdown-drain-e2e.test.tsShutdownDrainsurvives a real worker restart; stalled drains are force-killed at the configured ceiling (replication.blobSendDrainTimeout). PR#1621.P-350 (qa551 static root-mount / harper#1766) was dropped: already covered by the existing "static plugin with root urlPath (#1766)" suite in
components/static-urlpath.test.ts.All 4 promoted tests passed twice on
3b59214bb(v5.2.0-alpha.5) before promotion. One fixture file (registerCustody.jsfor P-328) required a depth-correction fix for thedist/relative path when moving fromqa-scratch/(3 levels deep) tocomponents/fixtures/(4 levels). Thestatic-after-rest-misconfigfixture was reconstructed from the warning-trigger logic inserver/static.ts(the snapshot only packaged the main fixture dir).Generated by Claude Sonnet 4.6.
🤖 Generated with Claude Code