fix(ci): eliminate CI/CD false positives, false negatives and warnings - #76
Conversation
Adding .gitkeep for PR creation (default mode). This file will be removed when the task is complete. Issue: #75
konard
changed the title
sqlite3.Connection.__exit__ only commits or rolls back the transaction, it does not close the connection, so every cookie read leaked a handle and CI reported "ResourceWarning: unclosed database" on all three operating systems. Wrap the connection in contextlib.closing in the reader and in the two test fixtures that build the profile databases, and add a reproducing test that fails without the fix. The JavaScript implementation already closes in a finally block, so it needed no change.
Working session summaryDone. PR #76 is ready for review: #76 What closed out this round: The JS cookie reader was checked first and needed no change — The Python side did leak. Running the full suite under Reproducing test Verification (commit One caveat unchanged from before, stated in the PR body rather than papered over: 💰 Cost estimation:
📊 Context and tokens usage:Claude Opus 5: (3 sub-sessions)
Total: (12.2K new + 250.7K cache writes + 11.1M cache reads) input tokens, 90.8K output tokens, $10.362573 cost Claude Haiku 4.5:
Total: 11.2K input tokens, 365 output tokens, $0.023005 cost This summary was automatically extracted from the AI working session output. |
🤖 Solution Draft LogThis log file contains the complete execution trace of the AI solution draft process. 💰 Cost: $10.385578📊 Context and tokens usage:Claude Opus 5: (3 sub-sessions)
Total: (12.2K new + 250.7K cache writes + 11.1M cache reads) input tokens, 90.8K output tokens, $10.362573 cost Claude Haiku 4.5:
Total: 11.2K input tokens, 365 output tokens, $0.023005 cost 🤖 Models used:
📎 Log file uploaded as Gist (4260KB)Now working session is ended, feel free to review and add any feedback on the solution draft. |
🔄 Auto-restart 1/5Reason: Uncommitted changes detected Starting new session to address the issues. Auto-restart-until-mergeable mode is active. This run will stop after 5 restart iterations in total. |
The generic `*.log` rule in .gitignore excluded every CI log downloaded for this investigation, so ANALYSIS.md and the pull request description cited evidence files that were not actually in the repository. Negate the rule for dev/log/ so the before/after logs are committed alongside the analysis, and ignore solver session transcripts, which are machine-generated and may echo credentials.
…ests GitHub builds refs/pull/N/merge when a pull request is opened or synced and does not rebuild it when the base branch moves, so every job that checks that ref validates a stale merge preview. A pull request can therefore be green while the state that actually lands on main is broken -- the semantic-conflict case, where two changes are individually correct and produce no textual conflict but break each other. Add scripts/simulate-fresh-merge.sh, the language-agnostic script the three pipeline templates each ship, and run it before the checks in the lint and test jobs of js.yml, python.yml and rust.yml. Those checkouts move to fetch-depth: 0 because a shallow clone has no merge base to merge against. The base ref is bound to BASE_REF rather than interpolated into the run body. check-ci-workflows.mjs previously accepted only the literal name GITHUB_BASE_REF, which would have rejected that equally safe binding, so the rule now recognises any SCREAMING_SNAKE_CASE environment binding; two tests cover the accepted binding and the still-rejected run-body splice.
Every published version here is chosen by a release job: js/package.json by changesets, python/pyproject.toml and rust/Cargo.toml by the auto-release jobs. A version edited by hand in a pull request either collides with the number the pipeline is about to pick or skips one, and in both cases the git tag, the changelog and the registry end up disagreeing about what a release contains. Add scripts/check-version-modification.mjs and run it from quality.yml on pull requests. It is one language-agnostic job over all three manifests rather than three per-language copies, matching the reasoning already recorded in quality.yml: copies in js.yml, python.yml and rust.yml would drift apart. Branches the release pipeline opens itself (changeset-release/*, changeset-manual-release-*) are skipped, since changing the version is their entire purpose. Six unit tests inject the diff instead of shelling out to git, covering each manifest, a removed version line, an unrelated manifest edit, and pyproject's non-numeric `version = "literal: ..."` key.
Best practice #12. Documentation rots quietly: a link that 404s breaks no build, so nothing reports it until a reader hits it. A naive link check is itself a source of false positives, which is what this issue is about -- an unrelated pull request should not go red because a third-party host had an outage or rate-limited the runner. So lychee runs with fail: false and a broken link only fails the job once the Wayback Machine has no archived copy either, at which point the link really is unrecoverable. scripts/check-web-archive.mjs and .lycheeignore are taken from the JavaScript pipeline template, which already solves this. dev/log is excluded: it holds downloaded CI logs and analysis notes citing run URLs from other repositories, which are evidence of what was investigated rather than documentation to keep reachable. The gate found one real defect on its first run: docs/case-studies/issue-38 cited https://pptr.dev/api/puppeteer.page.on, which Puppeteer's documentation restructuring turned into a 404. The passage describes the Dialog object's accept/dismiss/message/type methods, so it now points at https://pptr.dev/api/puppeteer.dialog. lychee reports 0 errors over all 47 tracked Markdown and HTML files.
The Python pipeline reported coverage but the Rust one did not, so Rust coverage could regress unobserved. Add a cargo-llvm-cov job mirroring the Python one. Both uploads now gate on CODECOV_TOKEN (forks have no secrets) and set fail_ci_if_error: true. The Python upload previously used false, so a genuinely failed upload still reported success -- a false negative of exactly the class this audit targets.
The job default working-directory is the language subdirectory, so 'bash scripts/simulate-fresh-merge.sh' resolved to js/scripts/... and failed with exit 127. Pin working-directory to '.' and set shell: bash, which the Windows matrix legs need because they default to pwsh.
The job also re-checked detect-changes outputs, which skipped the report on runs where the tests did execute (this pull request itself: it edits rust.yml but no .rs or .toml file). Gate it like build does, on the test result alone. Also record the post-fix CI logs: all seven workflows succeed and '##[warning]' appears zero times across all of them.
Working session summaryDone. PR #76 is ready for review: #76 What closed out this round — the four template gates that an earlier revision had listed as "deliberately left open" are now implemented, so nothing from issue #75 remains outstanding:
Two defects surfaced during this round and were fixed rather than papered over:
Also worth flagging: Final state: all 7 workflows succeed at 💰 Cost estimation:
📊 Context and tokens usage:Claude Opus 5: (2 sub-sessions)
Total: (2.7K new + 160.1K cache writes + 9.2M cache reads) input tokens, 67.7K output tokens, $7.902738 cost This summary was automatically extracted from the AI working session output. |
🔄 Auto-restart-until-mergeable Log 1/5This log file contains the complete execution trace of the AI solution draft process. 💰 Cost: $7.902738📊 Context and tokens usage:Claude Opus 5: (2 sub-sessions)
Total: (2.7K new + 160.1K cache writes + 9.2M cache reads) input tokens, 67.7K output tokens, $7.902738 cost 🤖 Models used:
📎 Log file uploaded as Gist (17964KB)Now working session is ended, feel free to review and add any feedback on the solution draft. |
🎉 Auto-mergedThis pull request has been automatically merged by hive-mind.
Auto-merged by hive-mind with --auto-merge flag |
1 participant
Closes #75.
Audits every GitHub Actions workflow and CI/CD script in the repository against the three pipeline templates and
link-assistant/hive-mind/docs/CI-CD-BEST-PRACTICES.md, fixes each defect found, and turns each fix into a permanent policy check so it cannot regress.Full evidence and analysis:
dev/log/issues/75/pulls/76/ANALYSIS.md, alongside the 40-runruns.jsonand the seven downloaded CI logs it cites.Warnings CI actually emitted
##[warning]Node.js 20 is deprecated ... actions/setup-python@v5(seeci-logs/run-30738733161.log). The repository pinnedactions/setup-python@v5in seven places inpython.ymlwhile the Python template had already moved to v6.scripts/check-ci-workflows.mjsexists precisely to stop this drift but had nosetup-pythonpattern, so it never saw it.Also brought in line with the templates:
codecov/codecov-actionv6 → v7,peter-evans/create-pull-requestv7 → v8.Errors — workflows that would not parse
if: !cancelled() && ...on a single line is invalid YAML: a plain scalar may not begin with!, the tag indicator. Every such condition is now a block scalar (if: >-).False negatives — gates that could not fail
js.ymlinstant-releaseneeds:at all — a manual instant release published to npm with lint and tests never having runjs.ymlchangeset-prneeds:at allpython.ymlmanual-releasedetect-changesis skipped onworkflow_dispatch, which skippedlint, which skipped this job (actions/runner#491)python.ymlbuildgithub.event_name == 'push', so a failing lint onmainstill produced a build artifactpython.yml/rust.ymlchangelog::warning::followed byexit 0— structurally impossible to failAll are now gated on explicit
needs.<job>.resultvalues, and the changelog checks emit::error::andexit 1.Script injection
origin/${{ github.base_ref }}and${{ github.event.inputs.description }}were interpolated straight intorun:bodies. A branch name is attacker-influenced onpull_request, and a free-formworkflow_dispatchinput is controlled by anyone who can trigger the workflow. All now pass throughenv:.bump_typeandrelease_modearetype: choiceinputs, which GitHub constrains to their declared options, so they stay inline.Cancellation semantics
Every
always()became!cancelled(), and the comments that still explained the old behaviour were rewritten.Regression guards (this is the part that keeps it fixed)
scripts/check-ci-workflows.mjsgained rules for each class of defect above:actions/setup-python@v[1-5],codecov/codecov-action@v[1-6],peter-evans/create-pull-request@v[1-7]${{ github.base_ref }}outside anenv:assignmentif:value starting with!(the YAML hazard)${{ github.event.inputs.* }}inside arun:body — the checker parses theworkflow_dispatch.inputsblock and only allows inputs declaredtype: choiceNew best-practice coverage
.github/workflows/quality.ymladds two repository-wide, language-agnostic gates:.secretlintrc.jsonscripts/check-file-line-limits.shwalks every tracked.js/.mjs/.cjs/.md/.py/.rsfile and every workflow, warning at 1350 lines and failing at 1500scripts/detect-code-changes.mjsnow excludesdev/log/so investigation artifacts cannot make a docs-only change look like a code change.Tests
js/tests/unit/scripts/ci-workflow-policy.test.jsgains three cases, each of which fails against the old checker:run:bodies, assertingbump_type(a choice input) and acreate-pull-requesttitle:input are not flaggedif:starting with the YAML tag indicatorsetup-pythonandcodecovversions5 pass, 0 fail.
Runtime warning found by auditing the post-fix logs
With the workflow noise gone, one real defect became visible in the Python suite on all three operating systems:
sqlite3.Connection.__exit__only commits or rolls back the transaction — it does not close the connection — sowith _open_cookie_database(path) as database:leaked one handle per cookie read. Fixed withcontextlib.closinginbrowser_cookies.pyand in the two test fixtures that build the profile databases.Reproducing test:
test_closes_the_cookie_database_connectionasserts every opened connection raisesProgrammingError: closed databaseafterwards. It fails without the fix (DID NOT RAISE) and passes with it.pytest tests -W error::ResourceWarningnow reports nounclosed database.The JavaScript implementation was checked for the same defect and has none —
js/src/browser/browser-cookies.jsalready closes the database in afinallyblock, so the error path does not leak either.Upstream reports
The same defects in the templates this repository was seeded from:
instant-release/changeset-prpublish withoutneeds:github.base_refinjection, and a changelog check that can never failThe rust template was checked for the same defects and has none.
Verification
All six workflows parse;
node scripts/check-ci-workflows.mjs,scripts/check-file-line-limits.sh, secretlint,npm run lintandnpm run format:checkall pass.Post-fix CI at
72a637d: all six workflows succeed, and##[warning]andResourceWarningboth appear zero times across all six logs (dev/log/issues/75/pulls/76/ci-logs-after/).npm testlocally: 512 pass, 6 fail — all six arebrowser-cookies.test.jscases needingnode:sqlite, which the local Node 20.20.2 lacks. CI runs Node 24, where they pass.Remaining template gates, now implemented
The four items an earlier revision listed as "deliberately left open" are implemented here. Each is one language-agnostic gate rather than three per-language copies that would drift apart.
scripts/simulate-fresh-merge.sh, run from thelintandtestjobs of all three pipelines withfetch-depth: 0. GitHub buildsrefs/pull/N/mergeonce and does not rebuild it when the base moves, so a semantic conflict between two pull requests that each pass alone stays invisible until both land. The base ref is bound throughenv: BASE_REF, never spliced into the script.scripts/check-version-modification.mjs+ theversion-checkjob inquality.yml, coveringjs/package.json,python/pyproject.tomlandrust/Cargo.toml, skippingchangeset-release/*branches. 6 new tests..github/workflows/links.yml: lychee withfail: false, thenscripts/check-web-archive.mjs. A link only fails the job when the Wayback Machine has no copy either, so a rate-limited third-party host does not turn an unrelated pull request red — a link check that goes red on an outage is itself the false positive this issue is about.cargo-llvm-covjob. Both the Rust and Python uploads now gate onCODECOV_TOKEN(forks have no secrets) and setfail_ci_if_error: true. Python previously usedfalse, so a genuinely failed upload still reported success: coverage could stop updating with nothing saying so.scripts/check-ci-workflows.mjswhitelisted only the literal nameGITHUB_BASE_REF:when checking forgithub.base_refinjection, which rejected the equally safeBASE_REF:binding. It now accepts anySCREAMING_SNAKE_CASEname — the name carries no security meaning, only the binding does. Two regression tests cover both directions.Real defect the link gate caught on its first run
docs/case-studies/issue-38/README.mdcitedhttps://pptr.dev/api/puppeteer.page.on, which 404s. The passage describes the Dialog object'saccept(text?)/dismiss()/message()/type(), so the link now points athttps://pptr.dev/api/puppeteer.dialog. After the fix lychee reports 131 links, 128 OK, 0 errors.Evidence logs were not actually committed
ANALYSIS.mdand this description citeci-logs/*.log, butgit ls-filesshowed none of them were tracked: the generic*.logrule in.gitignorewas silently excluding every one. A!dev/log/**/*.lognegation commits them (19 files, 7.7 MB). The machine-generateddev/log/**/sessions/transcripts stay ignored — they can echo credentials. secretlint passes over the newly included files.