diff --git a/.github/dependabot.yml b/.github/dependabot.yml index ea1b1e7..72bf1e8 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -1,9 +1,81 @@ # Dependabot: keep GitHub Actions on the latest versions automatically. # -# Repo convention: pin every action to the latest major (checkout@v7, -# upload-artifact@v7, ...). Watching upstream releases by hand leaks; Dependabot -# folds stale pins into one grouped PR per week, and merges are backstopped by -# the normal test suite. Major jumps like v7 -> v8 arrive in the same stream. +# Repo convention: pin every external action to a full 40-character commit +# SHA, with the release tag in a trailing comment +# (actions/checkout@ # v7.0.1). A mutable tag is a supply-chain +# liability: whoever controls the action's repository can repoint the tag at +# malicious code, and our workflows hold a token that can write to the +# public Homebrew tap that users install from. Pinning to a SHA makes the +# reference immutable, which is what GitHub's own hardening guide calls the +# most secure option, and what the OpenSSF Scorecard Pinned-Dependencies +# check measures. +# +# The local reusable-workflow call (uses: ./.github/workflows/update-tap.yml) +# is deliberately left unpinned: it is a path inside this repository, so +# there is no third-party source to pin, and GitHub requires local +# references to stay as paths. +# +# Why Dependabot still matters with SHA pins: Dependabot resolves the +# pinned action's latest release and opens a PR that rewrites the SHA and +# the trailing comment. Watching upstream releases by hand leaks; Dependabot +# folds stale pins into one grouped PR per week, and merges are backstopped +# by the normal test suite. Major bumps like v7 -> v8 arrive in the same +# stream. +# +# The known cost of SHA pinning, stated plainly so nobody rediscovers it: +# Dependabot ALERTS are only generated for actions referenced with semantic +# versioning, not SHA versioning. So after this convention lands, a +# compromised release of a pinned action arrives as a version-update PR +# rather than as a security alert, and dependabot_security_updates will +# have nothing to open for the github-actions ecosystem. That is the +# intended trade: a version-update PR still lands the fix, and the +# alternative (a mutable tag) means a retagged release reaches this repo +# without any PR at all. +# +# No alert channel remains for the action pins themselves. codeql.yml +# analyzes C++ only, not workflow files, so it is not a substitute here +# either. The version-update PR below is the only place a compromised +# action release would be seen, which is why the merge procedure below +# treats reading that diff as mandatory. +# +# Groups: one group for the whole ecosystem, so a week with several stale +# pins is one PR to review rather than one per action. +# +# MERGING A DEPENDABOT PR (the procedure, since it is not the obvious one): +# +# 1. Use the MERGE COMMIT strategy. Squash and rebase both put a commit on +# main that is authored by dependabot[bot], and a workflow on such a +# commit gets a read-only GITHUB_TOKEN, so the CodeQL SARIF upload fails +# with "403: Resource not accessible by integration". codeql.yml skips +# that upload so the run stays green, but a merge commit keeps publishing +# results. +# +# 2. The PR arrives with zero reviews and is BLOCKED on "review required". +# That is expected: Dependabot cannot approve its own PR. You bypass it +# the same way you bypass your own PRs, because the "Require review" +# ruleset lists the admin role as a bypass actor: +# gh api repos/justsrc/TetherKit/pulls//merge -X PUT -f merge_method=merge +# The REST endpoint makes the mechanism explicit. `gh pr merge` refuses +# at its own pre-flight when the PR is BLOCKED (cli/cli#13388, still +# open) and suggests --auto or --admin; --admin only suppresses that +# client-side refusal (no admin flag travels to the server, and the +# "Protect main" ruleset has no bypass actor at all, so nothing can +# override the required status checks). What merges the PR is your role: +# the "Require review" ruleset lists the admin role as a bypass actor +# with bypass_mode pull_request. +# +# 3. Read the diff before merging. The whole point of the SHA convention is +# that a new action version is a decision, not an automatic consequence: +# a version-update PR is the last human checkpoint before a new release of +# a third-party action can run in workflows that hold the tap token. +# A compromised release of a pinned action arrives here as an ordinary +# version-update PR precisely because SHA pinning kills the alert channel +# (see above), so this review is the only place that compromise would be +# caught. +# +# 4. Do NOT add Dependabot to the bypass list of any ruleset: that would +# let the bot merge its own PR unreviewed, which is exactly what the +# SHA pinning above exists to prevent. version: 2 updates: diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index def532a..70838e2 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -26,7 +26,7 @@ jobs: runs-on: macos-26 timeout-minutes: 30 steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install libusb run: brew install libusb @@ -46,7 +46,7 @@ jobs: ./build-rel/bin/tetherkit_bench | tee bench-report.md - name: Upload the bench report - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 with: name: tetherkit-bench-${{ github.run_number }} path: bench-report.md diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 9563dab..73c1650 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -44,7 +44,7 @@ jobs: DISPATCHED_BUILD_TYPE: ${{ inputs.build_type }} steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install libusb run: brew install libusb @@ -116,18 +116,18 @@ jobs: name="TetherKit-${GITHUB_RUN_NUMBER}-${DISPATCHED_OS}-${DISPATCHED_BUILD_TYPE}.zip" ditto -c -k --keepParent dist/TetherKit.app "$name" - # upload-artifact@v7 supports single-file direct upload (archive: false): + # upload-artifact v7 supports single-file direct upload (archive: false): # the downloaded artifact IS the ditto zip above, not a zip inside another # zip. With archive: false the name input is ignored (the file name wins). - name: Upload TetherKit.app artifact - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 with: path: TetherKit-${{ github.run_number }}-${{ env.DISPATCHED_OS }}-${{ env.DISPATCHED_BUILD_TYPE }}.zip archive: false retention-days: 14 - name: Upload libtetherkit shared-library artifact - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 with: name: tetherkit-libs-${{ github.run_number }}-${{ env.DISPATCHED_OS }}-${{ env.DISPATCHED_BUILD_TYPE }} path: build/lib/libtetherkit*.dylib diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 1df6747..b9119fb 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -7,23 +7,38 @@ # in the libusb transfer pool, the BPF buffer walk, and the # reinterpret_cast-heavy private ABI bridge are exactly its turf. # -# third_party/doctest stays in the analysis set on purpose: paths-ignore is -# inert for C/C++ manual builds (github/codeql-action#3890; the docs confine -# paths filtering to build-mode none, and the workflow would warn about it at -# run time). SARIF post-filtering means a new third-party action for exactly -# one benign vendored-code alert (cpp/use-after-free at -# third_party/doctest/doctest.h:3710). Accepted as baseline noise instead. -# # Free for public repos. Must run on macOS: CMakeLists fatals on non-Apple # platforms. # -# pull_request trigger restricted to workflow edits (paths: -# ['.github/workflows/**']): running a many-minute C++ build on every PR push -# isn't worth it, but workflow changes self-verify before landing. dependabot.yml -# and .github/scripts/ edits never match this filter; note it only gates -# pull_request runs, pushes to main still analyze them. Dependabot's weekly -# grouped action-bump PR deliberately rides along because it edits workflow -# files. The concurrency group below is event-scoped (the tests.yml doctrine): +# Merging a Dependabot PR (how this interacts with the read-only token): +# Prefer the MERGE COMMIT strategy. Squash and rebase both produce a main +# tip authored by dependabot[bot], and any workflow running on such a +# commit gets a read-only GITHUB_TOKEN, so the SARIF upload 403s. +# A merge commit is authored by the human merger and keeps write scope. +# The analyze step below also skips the upload for a Dependabot-authored +# push, so the run stays green either way, but the merge-commit path is the +# one that keeps publishing results. +# The review requirement on the bot PR is bypassed BY YOU, the same way it +# is for your own PRs: the "Require review" ruleset lists the admin role as +# a bypass actor with bypass_mode pull_request, so +# gh api repos/justsrc/TetherKit/pulls//merge -X PUT -f merge_method=merge +# engages the bypass. (`gh pr merge` refuses at its own pre-flight when +# mergeStateStatus is BLOCKED; that is cli/cli#13388, still open. The REST +# endpoint does the right thing.) Do NOT add Dependabot to the bypass list: +# that would let the bot merge its own PR unreviewed, which is exactly what +# the SHA pinning below exists to prevent. +# +# pull_request runs on every PR, not only on workflow edits. The old +# paths-filtered trigger was a cost decision (a many-minute C++ build per PR +# push) taken when the alert list was empty. It is now the wrong trade: the +# check's whole value is catching a defect BEFORE it lands, and the defects +# worth catching live in src/, not in .github/workflows/. With the filter, a +# pull_request run saw almost nothing, so "C++ analysis" could not be a +# required status check without deadlocking PRs that never trigger it. macOS +# minutes are free for public repos, so the cost is wall-clock on the PR, not +# money. +# +# The concurrency group below is event-scoped (the tests.yml doctrine): # push-to-main, the Monday cron and pull_request runs sit in different groups # and never cancel each other; within one event, canceling a superseded run is # safe because the newer run's SARIF simply replaces its alerts. @@ -37,8 +52,6 @@ on: push: branches: [main] pull_request: - paths: - - ".github/workflows/**" workflow_dispatch: permissions: @@ -61,12 +74,12 @@ jobs: name: C++ analysis runs-on: macos-26 steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install libusb run: brew install libusb - - uses: github/codeql-action/init@v4 + - uses: github/codeql-action/init@33d7b85da1a04171e211befe2c739303b9a4c066 # v4.39.3 with: languages: cpp # Manual build mode: the build commands come from the next step. @@ -74,8 +87,46 @@ jobs: - name: Build run: | - # Tests and benchmarks stay enabled: everything compiles, everything - # lands in the analysis set. + # Tests are OFF, so doctest never enters the analysis set. + # + # Why this mechanism and not the obvious ones: + # - paths / paths-ignore in a CodeQL config file are inert for C++ + # in manual build mode. The codeql-action maintainer states it + # directly (github/codeql-action#3890): "When using build-mode: + # autobuild or build-mode: manual, custom steps to ignore + # specified files are indeed still needed, and paths-ignore does + # not work." The docs confine path filtering to build-mode none. + # - SARIF post-filtering (advanced-security/filter-sarif) would work, + # but it is one more action to trust and pin on the analysis + # path, and it would need the workflow to upload the SARIF + # itself rather than letting the action do it. Not worth it for + # one benign vendored alert. + # - What GitHub does document for compiled languages is the right + # answer: "To restrict the analysis of a compiled language, you + # should configure your build process to compile only the + # specified files." For C++ the database contains exactly the + # translation units the build compiled. + # + # So the build decides the analysis set, and this project already + # has the switch: doctest is an INTERFACE library linked only into + # tetherkit_tests (tests/CMakeLists.txt), so with + # TETHERKIT_BUILD_TESTS=OFF no test TU is compiled, doctest.h is + # never parsed, and third_party/doctest never reaches the database. + # + # That retires cpp/use-after-free at third_party/doctest/doctest.h:3710 + # at the source instead of dismissing it. That alert is a false + # positive on a union assignment (the buffer is freed and a new one + # is assigned before the read; CodeQL's C++ model cannot see across + # the union member write), and it lives in a vendored test framework + # that never ships. Upstream doctest 2.5.3 carries the identical + # code, so upgrading does not clear it either. + # + # The cost, stated plainly: tests/ drops out of the analysis set + # along with doctest. That is first-party test code, not shipped + # code, and the shipped surface (src/, include/) stays fully + # analyzed -- which is where the BPF buffer walk and the + # reinterpret_cast-heavy private ABI bridge live, and those are + # exactly the findings worth having. # # CodeQL's macOS tracer forces traced child processes onto the x86_64 # slice under Rosetta (observed: build defaulted to x86_64; run 42513807606). @@ -88,9 +139,42 @@ jobs: # "simplification" would silently reintroduce the bug. cmake -S . -B build \ -DCMAKE_BUILD_TYPE=RelWithDebInfo \ - -DCMAKE_OSX_ARCHITECTURES=arm64 + -DCMAKE_OSX_ARCHITECTURES=arm64 \ + -DTETHERKIT_BUILD_TESTS=OFF cmake --build build -j"$(sysctl -n hw.ncpu)" - - uses: github/codeql-action/analyze@v4 + # The upload is skipped for a commit authored by Dependabot. + # + # Why: a Dependabot PR merged with SQUASH produces a commit on main + # that is authored by Dependabot, and any workflow running on such a + # commit gets a read-only GITHUB_TOKEN. The SARIF upload then fails + # with "403: Resource not accessible by integration" (the upload + # library warns and rethrows; the action's outer catch then fails the + # step via core.setFailed, so the run goes red). + # GitHub's own guidance is to merge bot PRs with a MERGE COMMIT instead, + # which keeps the commit authored by the human merger. This guard is + # the belt to that braces: it keeps the analysis running and the run + # green either way. + # + # The pull_request path is NOT affected and must not be guarded. Code + # scanning always allows uploading results when the pull_request event + # triggers the run, because the results are scoped to that one pull + # request. (upload-lib.ts's pull_request branch only fills base_ref and + # base_sha into the upload payload; the permission itself is GitHub's + # service-side policy.) Guarding it would be a regression: no upload + # means no result, and a required check that never reports blocks the + # PR forever (the failure mode documented in cli/cli#13388's + # neighbourhood: a scan that skips itself can never satisfy a rule + # that demands one). + # + # 'never' skips only the upload: the analysis still runs, the summary + # counts are printed into the job log, and the SARIF is written to + # ../results (outside the workspace, not uploaded and not kept as an + # artifact). This is the action's own input for the purpose + # (analyze/action.yml: upload: always | failure-only | never) at the + # pinned v4.39.3. + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@33d7b85da1a04171e211befe2c739303b9a4c066 # v4.39.3 with: category: "/language:cpp" + upload: ${{ github.event_name == 'push' && github.event.head_commit.author.username == 'dependabot[bot]' && 'never' || 'always' }} diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index b9f06f1..4d1113d 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -38,7 +38,7 @@ jobs: name: clang-format runs-on: macos-26 steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 # fetch-depth 2 provides the HEAD~1 fallback base below; anything older # (a pull-request base tip, or github.event.before on a batch push) is # fetched explicitly when missing. @@ -99,7 +99,7 @@ jobs: # continue-on-error to promote this to a hard gate. continue-on-error: true steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install libusb run: brew install libusb @@ -126,7 +126,7 @@ jobs: fi - name: Upload the clang-tidy log - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 if: always() with: name: tetherkit-tidy-log diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index dbc249d..4d44bf6 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -24,7 +24,7 @@ jobs: runs-on: macos-26 steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 # The tag and CMakeLists' VERSION must agree, otherwise # `tetherkit-cli --version` will not match the version on the Release / @@ -178,5 +178,10 @@ jobs: tag: ${{ github.ref_name }} # Narrow mapping instead of secrets: inherit: the callee needs exactly # one secret, and the caller's remaining secrets stay out of its scope. + # + # HOMEBREW_TAP_TOKEN is the fine-grained token scoped to + # justsrc/homebrew-tap (contents:write for the branch push, + # pull_requests:write for the PR). Declared in the callee's + # workflow_call secrets block, so the name validates at startup. secrets: - HOMEBREW_TAP_DEPLOY_KEY: ${{ secrets.HOMEBREW_TAP_DEPLOY_KEY }} + HOMEBREW_TAP_TOKEN: ${{ secrets.HOMEBREW_TAP_TOKEN }} diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 14379e8..d3b828b 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -4,11 +4,13 @@ # Split out of ci.yml. The five legacy jobs keep their steps and checks; later # additions are the ASan+UBSan job, the weekly schedule trigger, the # least-privilege permissions block, and the xcode-27 image legs. Deliberate -# deviations from the ci.yml text, four in all: actions/checkout is pinned to -# the latest major (v5 -> v7); the select-newest Xcode steps filter pre-release -# toolchains (beta and release candidate); the matrix os values carry the -# current image labels (macos-15, macos-26, xcode-27); and the abi-check grep -# evidence strings are English (this directory was translated wholesale). +# deviations from the ci.yml text, four in all: actions/checkout is pinned to a +# full commit SHA with the release tag in a trailing comment (the repo-wide +# convention, see .github/dependabot.yml); the select-newest Xcode steps filter +# pre-release toolchains (beta and release candidate); the matrix os values +# carry the current image labels (macos-15, macos-26, xcode-27); and the +# abi-check grep evidence strings are English (this directory was translated +# wholesale). # Triggers narrowed # from "push to main and swift-app branches + v* tags" to "push to main + PR + # manual + weekly schedule" because: @@ -84,7 +86,7 @@ jobs: build_type: [RelWithDebInfo, Release] steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Pin Xcode 27.1 on the xcode-27 leg if: matrix.os == 'xcode-27' @@ -145,7 +147,7 @@ jobs: name: Compile for macOS 13.3 runs-on: macos-26 steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install libusb run: brew install libusb @@ -185,7 +187,7 @@ jobs: os: [macos-15, macos-26, xcode-27] steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 # Select the newest Xcode in the image without trusting the image's # default: the original trigger was macos-14's Xcode 15.0 default, whose @@ -251,7 +253,7 @@ jobs: name: ThreadSanitizer runs-on: macos-26 steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install libusb run: brew install libusb @@ -283,7 +285,7 @@ jobs: name: ASan + UBSan runs-on: macos-26 steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install libusb run: brew install libusb @@ -368,7 +370,7 @@ jobs: os: [macos-15, macos-26, xcode-27] steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install libusb run: brew install libusb diff --git a/.github/workflows/update-tap.yml b/.github/workflows/update-tap.yml index 703ec41..6f814f9 100644 --- a/.github/workflows/update-tap.yml +++ b/.github/workflows/update-tap.yml @@ -6,10 +6,29 @@ # guard). A release-event hook would never fire. # So this workflow is workflow_call, invoked explicitly by release.yml. # -# Credentials: the tap carries a write-scoped deploy key; the private key -# lives in this repo's HOMEBREW_TAP_DEPLOY_KEY secret. GITHUB_TOKEN cannot -# cross repositories, which is why it has to be a deploy key. The key's scope -# is exactly that one repository, so a leak only exposes the tap. +# The tap update goes through a pull request, never a direct push to main: +# The tap is what users `brew install` from, so its contents are the highest +# value target in this pipeline. A direct push lands unreviewed. This +# workflow therefore pushes a bump branch and opens a PR; the maintainer +# merges it. The tap repo's own ruleset blocks direct pushes to main, so +# the PR is the only path. +# +# The release environment (declared on the job below) puts a human +# checkpoint in front of even creating the branch, so a compromised +# workflow cannot get a branch up without someone saying yes first. +# +# Credentials: one, HOMEBREW_TAP_TOKEN. A fine-grained token scoped to +# justsrc/homebrew-tap with contents:write and pull_requests:write. It does +# all three tap operations: the clone, the push of the bump branch, and the +# PR creation. GITHUB_TOKEN cannot cross repositories, which is why a token +# is needed at all; the token cannot reach any repository other than the tap. +# +# One credential covers all three operations, so there is one secret to +# provision and one to rotate. +# +# The token CAN write the tap's main directly. What stops it is the tap +# repo's own ruleset, which requires changes to land as a pull request. See +# AGENTS.md pitfall 26: the ruleset is the guarantee, not the credential. name: Update Homebrew Tap @@ -23,7 +42,7 @@ on: # Declared so release.yml's named secrets map validates: an undeclared # secret name fails a workflow_call at startup, with zero jobs and no logs. secrets: - HOMEBREW_TAP_DEPLOY_KEY: + HOMEBREW_TAP_TOKEN: required: true # For manual re-syncs: after a broken tap or a failed automatic sync. workflow_dispatch: @@ -33,7 +52,7 @@ on: required: true type: string -# The tap push happens over the scoped deploy key, so the GITHUB_TOKEN this +# The tap work happens over the scoped token, so the GITHUB_TOKEN this # workflow holds needs no write access anywhere in this repo. permissions: contents: read @@ -44,6 +63,10 @@ jobs: runs-on: macos-26 # Bounds a hung audit or build instead of burning the runner's minutes. timeout-minutes: 30 + # The human checkpoint in front of the tap. The environment carries a + # required-reviewer rule, so this job waits for a maintainer to approve + # the deployment before it creates the bump branch. + environment: release steps: - name: Compute the source tarball sha256 @@ -67,19 +90,15 @@ jobs: echo "version=${tag#v}" } >> "$GITHUB_OUTPUT" - - name: Set up the deploy key + # The tap is cloned over https with the token. One credential covers the + # clone, the push and the PR. + - name: Check out the tap env: - DEPLOY_KEY: ${{ secrets.HOMEBREW_TAP_DEPLOY_KEY }} + GH_TOKEN: ${{ secrets.HOMEBREW_TAP_TOKEN }} run: | # env passage, not direct expansion: a secret expression must never # sit inside run: text. - mkdir -p ~/.ssh - printf '%s\n' "$DEPLOY_KEY" > ~/.ssh/id_ed25519 - chmod 600 ~/.ssh/id_ed25519 - ssh-keyscan -t ed25519 github.com >> ~/.ssh/known_hosts 2>/dev/null - - - name: Check out the tap - run: git clone git@github.com:justsrc/homebrew-tap.git tap + gh repo clone justsrc/homebrew-tap tap - name: "Preflight: verify the formulas exist" working-directory: tap @@ -140,18 +159,127 @@ jobs: brew uninstall tetherkit tetherkit-cli - - name: Commit and push + # A branch and a pull request, never a push to main. The tap is what + # users install from, so its contents get a human read before they + # reach anybody's machine. + - name: Commit to a branch + id: commit working-directory: tap env: TAP_TAG: ${{ inputs.tag }} TAP_VERSION: ${{ steps.src.outputs.version }} + TAP_BRANCH: bump-tetherkit-${{ inputs.tag }} + # The credential helper reads the token from the environment when + # git asks for it, so it is passed as GH_TOKEN and never as part of + # a command line. This workflow's own GITHUB_TOKEN cannot write to + # the tap, so it must not shadow the tap token here. + GH_TOKEN: ${{ secrets.HOMEBREW_TAP_TOKEN }} run: | + # env passage, not direct expansion: a secret expression must never + # sit inside run: text. git config user.name "github-actions[bot]" git config user.email "41898282+github-actions[bot]@users.noreply.github.com" if git diff --quiet; then + # Already at this version. Say so and, crucially, tell the next + # step not to open a PR: without this output the PR step would + # run anyway and fail on a branch that was never pushed. echo "formula already at ${TAP_TAG}, nothing to do." + echo "changed=false" >> "$GITHUB_OUTPUT" exit 0 fi + # Branch first, then commit: the commit must land ON the branch. The + # opposite order commits on the checked-out default branch and only + # then creates the branch at that commit, which leaves the default + # branch itself carrying the change in the local clone. + # + # The branch name carries the tag, so a re-run for the same version + # lands on the same branch instead of stacking a second one. + git switch -c "${TAP_BRANCH}" git commit -am "tetherkit + tetherkit-cli ${TAP_VERSION}" - git push - echo "Tap synced to ${TAP_TAG}" >> "$GITHUB_STEP_SUMMARY" + # Authenticate the push with a credential helper rather than by + # putting the token in the remote URL. A credential in the URL is + # readable from .git/config, shows up in the process argument list, + # and would land in the CI log if the push failed. + # + # `gh auth git-credential` serves GH_TOKEN from the environment, and + # GH_TOKEN outranks GITHUB_TOKEN, so the push uses the tap token + # rather than this workflow's own GITHUB_TOKEN. The empty + # credential.helper resets any helper inherited from the runner's + # global config, so the only helper in play is the one below. + git config --local credential.helper "" + git config --local credential.https://github.com.helper \ + '!gh auth git-credential' + # --force: a re-run for the same tag rewrites the same branch, and + # the branch is this workflow's own scratch space. + git push --force origin "${TAP_BRANCH}" + echo "changed=true" >> "$GITHUB_OUTPUT" + echo "Bump branch ${TAP_BRANCH} pushed" >> "$GITHUB_STEP_SUMMARY" + + # Opening the PR is the API half, and it needs the same scoped token. + # + # The `changed` guard matters on a re-run for a tag that is already + # synced: the commit step exits early without pushing a branch, so there + # is nothing to open a PR from, and gh pr create would fail on a + # missing head branch. + + - name: Open the pull request + if: steps.commit.outputs.changed == 'true' + env: + TAP_TAG: ${{ inputs.tag }} + TAP_VERSION: ${{ steps.src.outputs.version }} + TAP_BRANCH: bump-tetherkit-${{ inputs.tag }} + GH_TOKEN: ${{ secrets.HOMEBREW_TAP_TOKEN }} + run: | + # env passage, not direct expansion: a secret expression must never + # sit inside run: text. + # + # Re-running for a version that already has a PR would fail with + # "a pull request for branch X already exists", so an open PR for + # this branch is treated as success. The branch push above already + # refreshed its content either way. + # + # The existence check is explicit rather than a bare `grep -q .`: + # gh prints a bare newline when nothing matches, and whether that + # counts as "something" depends on the grep implementation. (With + # the runner's BSD grep it does not match, so the old form worked + # here -- but only by that accident.) Testing for a non-empty + # number is the same check with no implementation dependence. + # + # It also fails loudly: if the gh call itself fails, the assignment + # takes the non-zero status and `set -e` ends the step there, so the + # error surfaces at the check instead of at a later `gh pr create`. + existing_pr="$(gh pr list --repo justsrc/homebrew-tap \ + --head "${TAP_BRANCH}" --state open --json number \ + --jq '.[0].number // empty')" + if [ -n "${existing_pr}" ]; then + echo "A pull request for ${TAP_BRANCH} is already open; nothing to do." + gh pr list --repo justsrc/homebrew-tap --head "${TAP_BRANCH}" \ + --state open --json url --jq '.[0].url' \ + >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi + + gh pr create --repo justsrc/homebrew-tap \ + --base main \ + --head "${TAP_BRANCH}" \ + --title "tetherkit + tetherkit-cli ${TAP_VERSION}" \ + --body "Automated tap bump for TetherKit ${TAP_TAG}. + + The formulas point at the ${TAP_TAG} source tarball; the sha256 was + recomputed from that tarball and both formulas were installed from + this working tree before the branch was created and pushed (brew + install --build-from-source, with codesign --verify --deep --strict + on the app). + + Checklist after merging: dispatch the tap's bottle.yml + (workflow_dispatch, formula: tetherkit, then tetherkit-cli) so the + new version's bottles are built, pushed to GHCR, and the formulas' + bottle blocks are updated. Until that runs, brew install for the new + version fails with a 404 on the GHCR manifest -- use + --build-from-source in the meantime. + + This PR needs a maintainer merge: the tap is what users install + from, so its contents do not change without a human reading them." + + gh pr list --repo justsrc/homebrew-tap --head "${TAP_BRANCH}" \ + --state open --json url --jq '.[0].url' >> "$GITHUB_STEP_SUMMARY" diff --git a/AGENTS.md b/AGENTS.md index 733773a..c1d71a6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -241,7 +241,7 @@ Legend: `Done` `In progress` `Not started` | 35 | `build(ci): split CI into tests, build, and lint workflows` | Done | ci.yml's five jobs moved into tests.yml with their steps and checks unchanged, triggers narrowed to PR + push main + manual (tag pushes covered by the self-sufficient release.yml; the swift-app branch is retired); new build.yml (workflow_dispatch only, runner-image and build-type inputs, .app and shared-library artifacts) and lint.yml (clang-format as a hard gate; clang-tidy advisory, since WarningsAsErrors: '' in .clang-tidy is an existing choice, promote it once the backlog is zero); all actions pinned to the latest major, checkout@v7 / upload-artifact@v7, with the .app uploaded via v7 single-file direct upload so the ditto zip arrives as itself | | 36 | `build(ci): add weekly regression, ASan/UBSan, CodeQL, benchmarks, and hardening` | Done | tests.yml gains a Monday cron (catches runner-image / new Xcode / latest libusb / xnu ABI drift before the next PR) and an ASan+UBSan job (splits the work with TSan: memory and UB vs concurrency; Sanitizers.cmake only forbids ASan+TSan together); new codeql.yml (github/codeql-action@v4, manual C++ build analysis, push main + cron, a hard complement to advisory lint) and bench.yml (dispatch-only benchmarks, Release without sanitizers, Markdown artifact, hosted-runner noise caveat); dependabot.yml bumps actions weekly in one grouped PR (backs the latest-major convention); tests/lint/build get permissions: contents: read; Xcode selection filters out beta versions | | 37 | `build(ci): translate release and tap workflows to English` | Done | release.yml and update-tap.yml translated wholesale (comments, step names, error echoes, and the user-facing Release notes heredoc); release.yml's checkout pin bumped v5 -> v7 per the latest-major convention; the two runtime strings the abi-check greps depend on (ReportSkip's root-skip message and the pair-creation MESSAGE in tests/test_net_link.cc) are now English literals, and tests.yml greps the matching strings byte-exactly (MESSAGE prints on success verified empirically against the vendored doctest 2.4.12) | -| 38 | `chore: point the tap, README links, and update checks at justsrc` | Done | update-tap.yml (clone url, local tap path, audit/install taps) and release.yml's Release-notes brew lines repointed to justsrc/tap; README.md and README.zh-CN.md install lines and tap link follow at that point; UpdateChecker's releases/latest endpoint and the READMEs' Releases links switched to fork releases per user decision, so the fork becomes the update source (pre-first-release the API 404s and the checker already surfaces that as Failure.noReleases); user-side steps outstanding: create justsrc/homebrew-tap, copy both formulas from the old tap and point their homepage at justsrc/TetherKit, add the public key as a write-scoped deploy key, store the private half as HOMEBREW_TAP_DEPLOY_KEY on justsrc/TetherKit | +| 38 | `chore: point the tap, README links, and update checks at justsrc` | Done | update-tap.yml (clone url, local tap path, audit/install taps) and release.yml's Release-notes brew lines repointed to justsrc/homebrew-tap; README.md and README.zh-CN.md install lines and tap link follow at that point; UpdateChecker's releases/latest endpoint and the READMEs' Releases links switched to fork releases per user decision, so the fork becomes the update source (pre-first-release the API 404s and the checker already surfaces that as Failure.noReleases); user-side steps outstanding at that time: create justsrc/homebrew-tap (since done), copy both formulas from the old tap and point their homepage at justsrc/TetherKit (still outstanding), and add the public key as a write-scoped deploy key + HOMEBREW_TAP_DEPLOY_KEY (superseded by row 60's one-token model: the tap now authenticates with the HOMEBREW_TAP_TOKEN repo secret) | | 39 | `fix(ci): survive the macOS 14 image retirement and rescope the format gate` | Done | Diagnosed PR #2's first runs (Tests 42487700197, Lint 42487700062, 2026-11-08): all four macos-14 jobs cancelled ~60s in with zero steps and the check-run annotation "The configuration 'macos-14-arm64-us-default' is not supported" -- GitHub retired the macos-14 runner image on 2026-11-02 (runner-images #13518); not fail-fast (matrices set fail-fast: false), not concurrency (no second run existed); Lint's clang-format job ran a repo-wide gate the repo could never satisfy -- the baseline is unformatted under current clang-format (~65 files, 491 violation lines in tests/test_net_link.cc alone). Fixes (commit b6135fd): macos-14 -> macos-15 in the three tests.yml matrices and build.yml's dispatch choice, with honest comment updates (macos-14 runtime-floor coverage ends; the minos-14.0 link flag and the Compile-for-13.3 job carry what they can); clang-format gate rescoped from repo-wide files to changed lines via vendored LLVM clang-format-diff.py (.github/scripts, run `-style file` explicitly because recent clang-format releases, 23.1.5 included, already default to file style and the flag survives a future default change (in sync with lint.yml's corrected comment)), diff base = PR base tip / event.before / HEAD~1 with a conditional base fetch and fetch-depth 2, plus an advisory repo-wide backlog count in the step summary; verified locally: positive probe (injected 200-col literal) exits 1, clean run exits 0, the branch's own hunks pass, and --dry-run without -Werror exits 0 so the summary always lands. A whole-file reformat of test_net_link.cc was tried and reverted: it moved 63 upstream Chinese lines (test titles/comments) into the diff for zero functional gain at the time | | 40 | `feat(ci): add xcode-27 image legs to the Tests matrices` | Done | Xcode 27 is not an /Applications entry on the macos-26 image -- under GitHub's one-major-Xcode-per-image model it ships as the separate label `xcode-27` (macOS 27 base, default toolchain Xcode 27.x; preview 2026-07-16, GA 2026-10-15, runner-images #14404; the macos-26 image tops out at 26.6 and macos-15 at 26.3, verified against both image Readmes); build-test, gui, and abi-check matrices therefore gain `xcode-27` as a third `os` value -- `runs-on: ${{ matrix.os }}` carries the label directly, no select step needed, names render as `xcode-27 · Release` / `GUI · xcode-27`; gui's select-newest step picked 27.x non-beta automatically at the time (the next commit replaces discovery on xcode-27 legs with a deliberate Xcode 27.1 pin: the image also ships Xcode 27.2.0 as a name-clean release-candidate symlink); abi-check gains it on purpose: the macOS 27 base means a new kernel/xnu, the exact surface where the FethPair private ABI could drift (the job's own long-term concern); tsan/asan-ubsan/deployment-target stay on macos-26 defaults with a pointed comment; new legs cost +4 macOS jobs per Tests run (11 -> 15), deliberately riding every PR (10x-billed macOS minutes accepted as the price of review-time drift catching); matrix entries documented with label provenance | | 41 | `fix(ci): make UBSan fatal and run the feth cases under it` | Done | Round-two review, B3. UBSan was report-and-continue: a diagnostic printed, the process kept going, ctest exited 0 through real undefined behavior. cmake/Sanitizers.cmake now appends -fno-sanitize-recover=undefined when TETHERKIT_ENABLE_UBSAN is set; the variable stays empty otherwise, so ASan-only and TSan builds get no extra flag (verified: a full configure with ASan+UBSan on writes -fno-sanitize-recover=undefined into compile_commands.json and flags.make, and c++ compiles a prober with the flag). The asan-ubsan Test step passes UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1 for sanitizer runtime pieces the compiler cannot touch, and runs the feth/BPF cases as root after net.link.fake.hwcsum=0; skipped root cases read green while the BPF buffer walk (bh_hdrlen, BPF_WORDALIGN) and the reinterpret_cast-heavy if_fake bridge never reach the sanitizer | @@ -263,12 +263,16 @@ Legend: `Done` `In progress` `Not started` | 57 | `feat(i18n)!: strip the Swift side to English-only, keep the add-a-language wiring` (this branch) | Done | Phase 2 complement to row 56. LocalizedStrings.swift went 743 -> 685 lines via a string-aware perl splice: every `localizations` switch row lost its Chinese first argument, with two guards - the dropped arg must contain Han or full-width punctuation OR be identical to the kept one (the languageEnglish native-name row needs the second rule), and the kept column must be Han- and fullwidth-free; kept spans are preserved byte-exact, including multi-line `+`-concatenation layouts (installer/update/permission strings); the file header and 42 MARK/comment lines went to English via an exact-line map; the `languageChinese` key is gone entirely (both Pickers hard-code `Text(verbatim: "English")` under the tag, so the key had no non-test consumers). Language enum is `.english` only with `cValue` constant 0; LanguagePreference loses `.chinese`, so a stale saved rawValue "chinese" decodes to nil and falls back to `.system` (graceful, symmetric with the C ABI ignoring unknown values); `L10n.apply` and `text(_:in:)` survive as the add-a-language dispatch points; new internal `L10n.localeLanguage` is parked always-English, exactly mirroring LanguageFromLocaleString on the C++ side, and `systemLanguage` still reads Locale.preferredLanguages through it; HelperService.setLanguage keeps its ignore-unknown semantics (an old App still sending "chinese" leaves the helper in English); both language menus now offer Follow system + English. **Swift constraint determined empirically** (tiny probe program): single-element labeled tuples are illegal - `cannot create a single-element tuple with an element label` - so the planned `(english: String)` one-member tuple is unrepresentable and rows are bare strings; when a language is added the property becomes a real labeled tuple and every case appends one labeled argument, the same way C++ rows `(id, en)` grow. LocalizationTests rewritten for the single table, still 7 cases: per-key placeholder contiguity, the %ld-integer rule, language resolution incl. localeLanguage, and formatting substitution; the cross-language placeholder/parity checks retire until a second language and the suite header says they return then. Verification: swift build green (3.3s, only the pre-existing libtetherkit dylib-version warning); swift test 30/30 - TetherKitIPCTests 17/17 (Authorization 2 + HelperConstants 8 + the 7 rewritten LocalizationTests) and TetherKitUIModelTests 13/13; the current-state snapshot below says "17 IPC + 7 UIModel = 24", which predates the sidebar test additions, not this work. Also folded in here: the row-56 typo fix (i18n.cci -> i18n.h). Splice/unwrap scripts live in the session temp dir. | | 58 | `feat(i18n)!: translate every Chinese comment and user-facing benchmark string to English` (this branch) | Done | P3 = the comment-and-doc-string half of the English-only migration (P1 = C++ message table, P2 = Swift string table). Scope: ~100 files, every Han comment in `include/`, `src/`, `tests/`, `benchmarks/`, `gui/` plus the benchmark tool's own report output; `docs/` + `README` + `AGENTS.md` are P4. Work was delegated in disjoint batches through the T3 `delegate_task` tool on the three OpenCode models the maintainer named (iceman-free, minimax-m3.1-flash-free, mimo-v2.6-flash-free; MiMo hit a free-tier limit mid-run and its remainder was re-dispatched to MiniMax); every batch was verified before acceptance, and three were sent back or fixed by hand. Verification method, all in-session: a comment-only scope proof (comments stripped from HEAD and from the tree, then the code token streams compared -- byte-identical for every file except the deliberately allowed literals), a comment-line-count drop check (a translation must not shrink a file's prose), a repo-wide Han census, the CI lint gate reproduced locally (vendored `.github/scripts/clang-format-diff.py` over the changed hunks, base HEAD), a full `-Werror` build plus 23/23 ctest, and `swift build`/`swift test` for the GUI batches. Findings worth keeping: (1) the house style is **pure ASCII in comments** -- the earlier crew had already converted box-drawing to `+ - |` in `feth_device.h` and dropped the `star`/`warning` markers, so two batches that kept them were normalized afterwards (emphasis now lives in the prose); (2) the CI format gate is **line-scoped**, so touching a test title drags its whole `TEST_SUITE` block into scope and clang-format then wants it re-indented by 2 -- the Chinese-era test files had their cases at column 0 and were never format-clean; the reformat was applied with an invariant that the **code token stream** is unchanged (a re-indent or a re-wrap may move a line break, never a token) and that the **comment word stream** is unchanged (a long comment may be reflowed, never reworded); (3) `src/app/main.cc` had to have its include blocks reordered (project headers before system headers, per `.clang-format`) -- pre-existing baseline debt that only became visible once a comment above it changed; (4) `docs/BENCHMARKS.md` is **half generated**: its first part is the literal output of `tetherkit_bench`, so the tool's report strings had to be translated too (they are user-facing) and the document's generated part must use exactly the wording the tool now prints -- but the document must NOT be regenerated, because that would drop the root-measured feth/BPF numbers and the hand-written analysis; (5) the `device` fixture in `tests/test_capi.cc` was **swapped, not translated**: it is a byte-pattern fixture (two 3-byte UTF-8 characters) and became two U+20AC characters (the euro sign, 3 bytes each, 6 bytes total) with every byte count in the assertions preserved; (6) the two CI grep anchors in `tests/test_net_link.cc` (the `ReportSkip` format string and the `created {} <-> {}` message) are byte-exact and must stay so; (7) a *passing* test is not proof -- a log-text needle such as `find("libusb initialized")` must match the production message table exactly or the assertion passes vacuously; (8) the Chinese strings in `tests/test_common_error.cc` are **paired** (a context string and the assertion on it) and were translated on both sides with identical wording, which is why the brief for that batch had to be corrected before dispatch. The GUI language plumbing, the XPC protocol revision 3 and the C ABI are untouched by this phase | | 59 | `docs(i18n)!: translate the documentation to English and drop the Chinese README` (this branch) | Done | P4 = the documentation half of the English-only migration (P3 = code comments). Scope: `AGENTS.md` (857 -> 488 lines), the six `docs/*.md` files, `README.md`, and the deletion of `README.zh-CN.md`. Delegated in four disjoint batches through the T3 `delegate_task` tool on the three OpenCode models the maintainer named; the README edits and the Chinese-README deletion were done by the maintainer. Verification, per file: a numeric-token multiset comparison against HEAD (every measurement preserved; the only differences are three correct `wan`-unit conversions in BENCHMARKS.md -- `124 wan` -> "1.24 million frames per second", `2.7 wan` -> "27,000 frames per second", `46 wan` -> "460,000 frames per second"), a fence-aware structural comparison (headings, table row counts and per-table pipe counts identical in every file), a link-resolution check, and a repo-wide Han scan that reaches zero. Findings worth keeping: (1) `docs/BENCHMARKS.md` is **half generated** -- its first part is the literal output of `tetherkit_bench`, so it must use exactly the wording the tool now prints, and it must **not** be regenerated (that would drop the root-measured feth/BPF numbers and the hand-written analysis); the tool's English output was captured and used as the reference; (2) a document that quotes GUI strings must quote them **verbatim**: four quotes in GUI-ARCHITECTURE.md did not match the table ("Check for Updates..." vs the real "Check for Updates...", "Uninstall the privileged helper..." vs "...helper?", the invented "Install / update the privileged helper" button, and a `%ld` template rendered as a literal) and were corrected against `LocalizedStrings.swift`; (3) **ASCII-only yields to fidelity** when the text is a quotation of a real string or a literal that carries meaning: GUI-SPIKE.md keeps a full-width parenthesis in its bash snippet because the documented `0xEF` lead-byte trap depends on it (the reproduction was re-verified: `IFACE?: unbound variable`), and the two UI titles above keep the app's real ellipsis and question mark; (4) the AGENTS.md ledger rows and the pitfall list keep their exact numbering (the 58 pre-existing rows, plus this one; pitfalls 1..24 plus 4b), so the code's citation "AGENTS.md section 7, pitfalls 12 and 13" still resolves to the same two pitfalls; the emoji status marks became the word "Done" table-wide (row 58 included, its prose untouched), and the stale test count was corrected from 24 to the measured 30 (17 TetherKitIPC + 13 TetherKitUIModel); (5) the AGENTS.md directory tree no longer lists the deleted `README.zh-CN.md`, while ledger row 38 keeps the mention as a historical record of what that commit did; (6) the i18n section was adapted to the single-language reality while naming the retained plumbing and its five-touch checklist. Nothing pushed | +| 60 | `build(ci): pin actions, widen CodeQL, open a PR for tap updates` (this branch) | Done | Supply-chain and release-gate hardening, all three parts researched against current upstream docs before writing. **SHA pinning**: every external action reference (18 across 6 workflow files) is now a full 40-char commit SHA with the release tag in a trailing comment (actions/checkout@3d3c42e # v7.0.1, actions/upload-artifact@cf430e0 # v7.0.2, github/codeql-action@33d7b85 # v4.39.3); SHAs were resolved through the git-ref API, dereferenced for annotated tags, then each confirmed to BE the release commit (checkout = 'prep v7.0.1 release', upload-artifact = the v7.0.2 merge, codeql-action v4 == v4.39.3 by compare). Rationale: a mutable tag can be repointed at malicious code and our workflows hold a token that writes the public tap users install from; SHA pinning is what GitHub's hardening guide calls the most secure option and what the OpenSSF Scorecard Pinned-Dependencies check measures. **The documented cost, recorded so it is not rediscovered**: Dependabot ALERTS are only generated for actions referenced with semver, not SHA (docs: 'alerts are only generated for actions that use semantic versioning, not SHA versioning'), so a compromised release of a pinned action now arrives as a version-update PR rather than a security alert, and dependabot_security_updates has nothing to open for the github-actions ecosystem. dependabot.yml was rewritten to state the new convention and that trade. The local reusable-workflow call (uses: ./.github/workflows/update-tap.yml) stays a path on purpose. **CodeQL**: the pull_request paths filter was removed (row 55 had narrowed it to .github/workflows/**); it now runs on every PR, because the check's value is catching a defect before it lands, and with the filter 'C++ analysis' could not be a required status check without deadlocking PRs that never trigger it -- macOS minutes are free for public repos, so the cost is wall-clock, not money. The vendored doctest is now excluded from the analysis set by BUILDING less, not by path filters: -DTETHERKIT_BUILD_TESTS=OFF, which is the mechanism the docs prescribe for compiled languages ('configure your build process to compile only the specified files'); paths-ignore is inert for C++ manual builds (codeql-action#3890, maintainer statement) and SARIF post-filtering via advanced-security/filter-sarif was declined rather than unavailable: the action is public, but it is one more third-party action to trust and pin on the analysis path, and it would need the workflow to upload the SARIF itself rather than letting the action do it -- not worth it for one benign vendored alert. That retires cpp/use-after-free at third_party/doctest/doctest.h:3710 at the SOURCE instead of dismissing it -- it is a union false positive (the buffer is freed and reassigned before the read) in a test framework that never ships, and upstream doctest 2.5.3 carries the identical code, so upgrading would not clear it. Cost: tests/ drops out of the analysis set with doctest; src/ and include/ stay fully analyzed, which is where the BPF buffer walk and the reinterpret_cast-heavy private ABI bridge live. **Release gate**: the tap update no longer pushes to main. It pushes a bump branch and opens a PR, because the tap is what users brew install from and its contents should get a human read. One credential, a fine-grained token scoped to justsrc/homebrew-tap with contents:write and pull_requests:write: it clones the tap over https, pushes the bump branch, and opens the PR (GITHUB_TOKEN cannot cross repositories, which is why a token is needed at all). The token reaches git through a credential helper rather than through the remote URL: the push step configures `credential.https://github.com.helper = !gh auth git-credential` in the clone's local config, and the helper reads the token from GH_TOKEN in the environment at the moment git asks for it, so the token never appears in a command line, in .git/config, or in a failure message. The 'release' environment on TetherKit carries a required-reviewer rule (justsrc), so the job waits for an approval before it even creates the branch; the tap repo got its own ruleset requiring a PR on main, so a direct push is impossible there, not merely avoided -- the token can write the tap's main directly and only the ruleset stops it. Verification, local only: every workflow parses (PyYAML), actionlint reports zero findings on the three substantively edited files and no new findings against the pre-edit baseline on the others (the xcode-27 runner-label and shellcheck notes are pre-existing), all eight run: blocks pass bash -n, and a cmake configure with -DTETHERKIT_BUILD_TESTS=OFF succeeds. Findings worth keeping: (1) 'commit.signoff' is NOT a git config key -- git 2.56 only knows format.signoff, which governs format-patch; git commit -s adds the trailer, the config does not, and the setting reads back fine while doing nothing, so the check that matters is the trailer's presence, not the config's value; (2) 'web_commit_signoff_required' governs commits made through the WEB interface only, so it never gated CLI pushes; (3) the repo's own history is 145 direct commits to main against 5 merges, so a required-checks rule on main changes the working style: direct pushes are rejected, and 'strict' (branch must be up to date with main) compounds that, which is why strict is off and the maintainer chose PR-only; (4) an open 'critical' code-scanning alert on vendored code is a reputational cost even when it is a false positive, which is the second reason to fix it at the source rather than dismiss it. Committed and pushed as feat/ci-hardening; see row 61 for the follow-up | +| 61 | `fix(ci): keep the CodeQL upload alive on Dependabot merges` (this branch) | Done | A Dependabot PR merged with SQUASH produces a commit on main that is AUTHORED BY DEPENDABOT, and any workflow on such a commit gets a read-only GITHUB_TOKEN, so the SARIF upload 403s ("Resource not accessible by integration"). The upload library logs a warning and rethrows (upload-lib.ts's 403 handler); the action's outer catch then fails the step via `core.setFailed`, so the run goes red. The analyze step now sets the action's own `upload` input to `never` for a Dependabot-authored push and `always` otherwise, so the run stays green while the analysis still runs; only the publish is skipped. The action's `getUploadValue` returns exactly always/failure-only/never (legacy `true`/`false` inputs map onto them; unknown values warn and default to always), and the 'never' branch skips the upload call and returns normally (read out of the action source at the pinned v4.39.3, not just the docs). **The pull_request path is deliberately NOT guarded**, which corrects an earlier assumption in this session: code scanning always allows uploading results when the pull_request event triggers the run, because the results are scoped to that one PR, and upload-lib.ts carries an explicit pull_request branch for it. Guarding it would be a regression, and a regression with teeth: no upload means no result, and a required check that never reports blocks the PR forever. **Two wrong turns worth recording**: the input is `upload`, not `upload-sarif` -- that boolean belongs to OSV-Scanner, and copying it here would have been silently ignored; and both `paths-ignore` and a `branches`+`branches-ignore` pair were wrong -- actionlint rejects a `branches`+`branches-ignore` PAIR on one event (paths filters combine with branches fine -- probe: exit 0 -- but paths-ignore is inert for C++ manual builds), and a squash-merged bot commit lands on main, so a dependabot/** branch filter cannot match it anyway. Verification, local only: the expression's four event/author combinations evaluated by hand, the workflow parses, actionlint is clean, the input name and its accepted values read out of the action.yml at the pinned SHA, and getUploadValue plus the 'never' branch read in the action source. Committed and pushed as feat/ci-hardening | +| 62 | `docs(ci): document the Dependabot PR merge procedure` (this branch) | Done | The SHA pinning made Dependabot a weekly event, so the procedure for merging its PR is now part of the repo's operating instructions rather than tribal knowledge. It lives in .github/dependabot.yml, where a maintainer opening a bot PR will look, and it covers: merge-commit rather than squash or rebase (both leave the tip authored by Dependabot), because a squash or rebase commit is authored by Dependabot and loses write scope (the merge commit is authored by the human, so results keep publishing); the REST bypass command for the review gate, `gh api repos/justsrc/TetherKit/pulls//merge -X PUT -f merge_method=merge`, because `gh pr merge` refuses at its own pre-flight when the PR is BLOCKED (cli/cli#13388, still open) and suggests --auto or --admin; --admin only suppresses that client-side refusal -- no admin flag travels to the server (gh's mergePayload has no such field) and the 'Protect main' ruleset has no bypass actor at all, so nothing can override the required status checks. The merge succeeds because the 'Require review' ruleset lists the admin role as a bypass actor with bypass_mode pull_request, which engages server-side on any PR merge by the admin, REST or gh; and the reminder that the version-update PR is the ONLY human checkpoint a compromised action release would meet, since SHA pinning deliberately kills the Dependabot alert channel (row 60). **The review requirement needed no settings change**, and the reason is the non-obvious part: the 'Require review' ruleset already lists the admin role as a bypass actor with bypass_mode pull_request, so the human who merges the bot PR bypasses the review exactly as for their own PRs. Adding Dependabot to the bypass list would have been actively wrong -- it would let the bot merge its own PR unreviewed, defeating the SHA pinning. That warning is carried in both dependabot.yml's step 4 and codeql.yml's header, so a maintainer meets it whichever of the two files they open. Also confirmed while writing it: no PR-triggered workflow (tests, lint, codeql) references a secret, so Dependabot's no-secrets rule is a non-issue for them. Committed and pushed as feat/ci-hardening | +| 63 | `docs(ci): apply the second-review text corrections` (this branch) | Done | A second, line-by-line review of rows 60-62 and the three CI files. Verdict: no defect in the workflows themselves -- every change is comment, prose, or a PR body -- but nine places where the ledger described the mechanism inaccurately or claimed something untrue, one addition pointing at where a warning now lives, and one check that turned out to be correct only by accident. **Every claim in the review was verified against upstream source before being applied, not taken on trust**, and one of them turned out to be overstated: the review says `gh pr merge --admin` "only suppresses that client-side refusal", but gh's own help and source show it also skips the merge queue client-side (the help text: "To bypass a merge queue and merge directly, pass the `--admin` flag"); the wording keeps the server-side half, which is the part that matters, and the load-bearing finding stands either way -- gh's `mergePayload` carries no admin field, so nothing about privileges travels to the server, and the 'Protect main' ruleset has `bypass_actors: []` with `current_user_can_bypass: never`, so no admin can override the required status checks. What merges the PR is the 'Require review' ruleset, which lists the admin role as a bypass actor with `bypass_mode: pull_request`. Corrections applied: row 38's open-loop deploy-key instruction became a closed loop (the tap repo is since done, the formula copy is still outstanding, and the deploy key is superseded by row 60's one-token model, so an onboarding agent can no longer act on a credential that is no longer used); row 60's claim that SARIF post-filtering "requires an Enterprise-owned repository" was **wrong on the facts** (the action is public) and was rewritten as a decision rather than a constraint -- declined, because it is one more third-party action to trust and pin on the analysis path; row 61's three clause fixes were corrected to the two-layer failure path (upload-lib.ts warns and rethrows, the action's outer catch then calls `core.setFailed`, read at analyze-action.ts:451 at the pinned v4.39.3), to `getUploadValue`'s real contract (exactly always/failure-only/never, with legacy `true`/`false` inputs mapped onto them and an unknown value warning and defaulting to always), and to actionlint's real rule (a `branches`+`branches-ignore` **pair** on one event is rejected, exit 1, while paths filters combine with branches fine, exit 0 -- both probed on actionlint 1.7.12); rows 61 and 62 ended with "Nothing pushed" although their commits are on origin/feat/ci-hardening (350eb0e and c34783e), so both now read "Committed and pushed as feat/ci-hardening"; row 62's merge strategy now says merge-commit rather than squash **or rebase** (both leave the tip authored by Dependabot) and its `--admin` paragraph was replaced with the verified mechanism. The one behavior change: update-tap.yml's existing-PR check piped `gh pr list --jq '.[0].number'` into `grep -q .`, and gh prints a bare newline when nothing matches -- **the old form was not actually broken** (probed: BSD grep, which the runner uses, does not match `.` against a bare newline, so the old check took the right branch in both cases), but it worked only by that accident, so the check now tests for a non-empty number explicitly and is independent of the grep implementation; probed live against a branch with an open PR (yields the number) and one without (yields empty). The rewrite also changes one failure mode: under `bash -e` (GitHub's default for `run:` blocks) a failing `gh` call now ends the step at the check, because the assignment takes the non-zero status, where the old pipeline fell through to `gh pr create` -- the error surfaces at the check instead of at a later command. Delegation: two disjoint scopes went to iceman-free subagents (codeql.yml, dependabot.yml), each verified independently afterwards by diffing the comment-stripped payload against HEAD (byte-identical), so no YAML key, expression, or action pin moved; update-tap.yml was edited directly, because it is the secret-bearing file and row 52's workspace-guard rule keeps non-primary providers out of it. Also documented, not fixed: the tap token's `contents:write` also reaches the tap's tags and releases, which no ruleset governs (pitfall 26). Committed, not pushed | ### Current state - **Tests**: all 23 ctest cases pass; on the GUI side `swift test` has 30 cases passing (TetherKitIPCTests 17 and TetherKitUIModelTests 13, including LocalizationTests and HelperConstantsTests). - **Build**: zero warnings under `-Werror`; the CLI, shared library, and GUI products all build. -- **CI**: legacy ci.yml split into tests.yml (five jobs migrated with their steps and checks unchanged, triggers PR + push main + Monday cron + ASan/UBSan), build.yml (manual trigger only, full build + artifacts) and lint.yml (clang-format gate + advisory clang-tidy); new codeql.yml (weekly + push main C++ scanning), bench.yml (manual benchmarks) and dependabot.yml (weekly grouped action bumps); permissions: contents: read across tests/lint/build; Xcode selection filters beta; every action pinned to the latest major (checkout@v7, upload-artifact@v7). +- **CI**: legacy ci.yml split into tests.yml (five jobs migrated with their steps and checks unchanged, triggers PR + push main + Monday cron + ASan/UBSan), build.yml (manual trigger only, full build + artifacts) and lint.yml (clang-format gate + advisory clang-tidy); new codeql.yml (weekly + push main + every-PR C++ scanning), bench.yml (manual benchmarks) and dependabot.yml (weekly grouped action bumps); permissions: contents: read across tests/lint/build; Xcode selection filters beta; every external action is pinned to a full commit SHA with the release tag in a trailing comment (see ledger row 60 for why SHA rather than a major tag). - **Runnable**: `--version` / `--help` / `--list` all work; starting without root prints a clear hint and returns exit code 1; `TetherKit.app` and `tetherkit-helper` both start after packaging; the privileged helper can be installed / uninstalled inside the App with one click (the install path has been exercised on real hardware); when the helper version does not match the App, the management row lights up "Update the helper" (it compares the **library** version, not Info.plist -- `brew upgrade` only replaces the .app, the copy installed under /Library does not follow; when the protocol revision did not change there was previously no prompt at all, and the only thing the user could notice was "the issue described as fixed in the release notes is still there"; the windows were screenshot-checked); English-only behavior is verified: CLI `--help` / `--list` / argument errors are correct, and switching the language in the GUI menu updates the main window and the App menu **immediately** (no restart required, screenshot-checked). - **Verified**: the USB side works on a real RNDIS device (enumeration, interface claim, RNDIS handshake), and this USB side **does not require root**; the feth private ABI, the two private BPF ioctls, and the core assumption that "a BPF `write()` lets the peer IP stack receive a frame" have all been confirmed on real hardware. See section 6. - **Benchmarked** (2026-07-28, Android USB tethering): end-to-end iperf3 measured **RX 324-327 Mbps / TX 234-241 Mbps** (USB 2.0 High Speed, theoretical ceiling 426 Mbps); the full GUI-to-real-device link works (plug in -> connect -> obtain a DHCP address -> bidirectional ping). See section 6.4 and [docs/BENCHMARKS.md](docs/BENCHMARKS.md). @@ -455,7 +459,47 @@ Warning: two statistics miscounts were fixed along with this: oversized frames a Round 9 (live recheck overturned r8's FKA-off Tab contract): (1) under keyboard navigation the Tab cycle originally did not enter the sidebar (row `.focusable(false)`), so as long as any control held focus, the "focus is empty/window itself" precondition for the window arrow-key claim failed -> Tab could not reach the sidebar and arrow keys were stranded (user live report). The contract reversed: row `.focusable(true)` became constant -- the r8 probe-matrix expectation "Tab never lands on a row" is obsolete from this point and the probe matrix must update its expectations first; FKA-off inertness no longer comes from the modifier chain but from activation-action branching: inside the FKA-off gate, native activation driven by the keyboard (live keyDown) is discarded and mouse events pass through, distinguished by `NSApp.currentEvent` type. Warning: `currentEvent` is read in two places (this gate and `ButtonTurn.write`'s defer decision), both only as a negative/branching signal -- an AXPress turn can leave a stale old event behind (this pitfall round 6), so do not use it as a positive predicate. (2) Return live-failure follow-up: the AX self-read claimed by the window's `performKeyEquivalent` could resolve a row in the synthetic probe but could not resolve one under the user's live FKA -> Return silently fell through. Added the focused row's own `.onKeyPress(.return)` (FKA-on gate, calling `rowSelect` directly), with zero AX-read dependency -- a headless environment cannot bypass the HID layer, so which link in this chain breaks on live hardware can only be verified on live hardware; the two claim levels are first-to-consume, and at worst the duplicate is an idempotent page switch. Numpad Enter (keycode 76) is still covered only by the window claim (SwiftUI `KeyPress.Key` has no numpad variant). Round 10 (self-driven keyboard-ring exploration, superseded in Round 11): Warning: Round 10 explored claiming bare Tab at the AppKit window layer, maintaining a self-driven keyboard ring in `AppModel.sidebarRingRow`, and using `SidebarFocusBridge` for probe sampling. That design was proven over-engineered and blocked the normal focus order in Round 11 and has been completely removed; no residue such as `sidebarRingRow` remains in the code, do not reintroduce it. Round 11 (final architecture: native focus-cycle traversal and arrow-key activation in all states): (1) Root cause identified: the dead end that kept the probe from entering the sidebar was the `.focusable(SelfAccessibility.isFullKeyboardAccessOn)` bound on the `rows` container (which marked the whole group unfocusable when FKA-off); focusable removed from the container, only the selected row declares `.focusable(isSelected)` when FKA-off (all rows focusable when FKA-on). The Tab key now forms a "selected sidebar row <-> content buttons" bidirectional native Tab cycle directly. (2) Arrow-key activation: the `handleArrowKey` guard was changed to pass through for text editing (cursor movement) only when `isFirstResponderEditableText` is true; when a button/card/scroll area has focus, arrow keys step pages directly at any time, page switches self-heal focus, and the user is never stuck on the current page. The container `.onKeyPress` keeps up/down arrow keys as a backstop. With FKA-on, arrow keys are taken over by the system focus engine (the HID-layer "Move Focus" keys), so neither the window claim nor the container backstop receives the events; arrow keys move key focus itself -- expected behavior, not a defect. (3) Return claim and row-level backstop: when the first responder is not a field editor, the window's `performKeyEquivalent` attempts an in-process system-level AX focus resolution (requires TCC trust); if untrusted or no row is resolved, the event falls through to the focused row's own `.onKeyPress(.return)` backstop to switch pages. (4) Explicit focus highlight: when any row has focus and the window is active (`controlActiveState == .key`) it shows an accent border and a light-gray background; when the window loses focus to the background the border is removed automatically. The selection follows arrow-key movement in sync. +25. **A required-status-check rule on `main` changes how you work, and `strict` makes it worse.** + The repo's own history is 145 direct commits to `main` against 5 merge commits, so direct pushes are the normal way work lands here. Adding `required_status_checks` to `main` rejects a direct push: the checks have to have passed on that commit *before* the push, which means the commit has to live on a branch first. That is not a subtle effect, it changes the workflow. `strict_required_status_checks_policy: true` compounds it: the topic branch must be up to date with `main`, so every PR needs a merge or rebase of `main` into it first, and the checks re-run. On a repo where `main` moves often, that is the difference between "merge my PR" and "merge `main` into my PR, wait for CI, then merge my PR". The maintainer's choice for this repo is PR-only, so `strict` is off and the checks are the loose kind: the branch does not have to be current, and the checks that ran against the branch head are what gate the merge. Warning: the required-check list is matched by **check-run name**, so a matrix-expanded name (`macos-26 · Release`, `feth private-ABI check · xcode-27`) is brittle -- adding an Xcode leg or a build type changes the names, and a required context that never reports blocks a PR forever with no useful error. The names here were taken from live check-runs rather than typed by hand, and they carry `integration_id: 15368` (github-actions) so no other app can satisfy them. When the matrix changes, update the ruleset's context list in the same commit. +26. **The token that writes the tap's `main` is not what stops it -- the tap's ruleset is.** + The tap update pushes to `justsrc/homebrew-tap`, a different repository, so `GITHUB_TOKEN` cannot reach it. One fine-grained token scoped to that tap does the whole job: clone over https, push the bump branch, open the PR. That token CAN write the tap's `main` directly, and nothing in the credential stops it. + + What stops it is the **tap repo's own ruleset**, which requires changes to land as a pull request. Proven, not assumed: with a write-scoped credential in hand, a direct push to the tap's `main` was rejected with "GH013: Repository rule violations found... Changes must be made through a pull request", and `main` did not move. + + So do not describe a credential split as the review guarantee. A credential controls the blast radius of a leak; the ruleset controls whether unreviewed code can reach the tap. Two different properties, two different mechanisms, and only one of them is the guarantee. + + One credential rather than two is the simpler shape: one secret to provision, one to rotate, one capability to reason about when it leaks. + + One residual is worth naming so nobody mistakes it for a bug: the token's `contents:write` also reaches the tap's **tags and releases**, which no ruleset governs (the tap's ruleset gates branches only). Fine-grained tokens have no narrower grant that still allows branch pushes plus PR creation plus file writes, so this cannot be tightened -- it can only be documented, and it is bounded by the token's own scope. + +27. **`commit.signoff` is not a git config key, and `web_commit_signoff_required` does not gate CLI pushes.** + Two adjacent traps that both look like "sign-off is on now". (1) `commit.signoff` does not exist: git 2.56's binary carries only `format.signoff` (which governs `format-patch`, not `git commit`). Setting it reads back cleanly and does nothing, while `git commit -s` does add the trailer. So the thing worth checking is whether the trailer is in the message, never whether the config key is set. (2) `web_commit_signoff_required` governs commits made through the **web interface** only, per the field name and GitHub's docs; it never rejects a CLI push. Enabling it is a two-minute change with no effect on a maintainer who pushes from the terminal, so do not read it as "sign-off is now enforced on my commits". + +28. **A token in a git remote URL leaks three ways, and only one of them is the file.** + + The pattern `git remote set-url origin https://x-access-token:$TOKEN@github.com/owner/repo.git` puts the credential in three places, and the obvious one is the least interesting: + + 1. **`.git/config`**, which persists for the life of the clone and is readable by anything that can read the file. + 2. **The process argument list**, where `ps` shows it to every user on the machine. + 3. **The failure message.** This is the one that bites in CI. A wrapper that raises `subprocess.CalledProcessError(returncode, command, ...)` embeds the whole command list in the exception, the traceback prints it, and the CI log prints the traceback. The token lands in the log, where it is not masked unless GitHub happens to know the exact string. + + Point 3 is why "we set the URL and then unset it again afterwards" is not a fix. The unset only protects the file; the argv exposure and the error path are untouched. + + The fix that removes all three is a **credential helper**: the token lives in the environment, git asks a helper for it at the moment it needs it, and the helper is the only thing that ever sees the value. + + git config --local credential.helper "" + git config --local credential.https://github.com.helper '!gh auth git-credential' + GH_TOKEN=$TOKEN git push + + Facts measured while fixing this, all against the real tap: + + - `gh auth git-credential get` serves `GH_TOKEN` from the environment, and `GH_TOKEN` **outranks** `GITHUB_TOKEN`, so the helper uses the tap token and not the workflow's own `GITHUB_TOKEN`. The precedence is the whole reason this works: the workflow's `GITHUB_TOKEN` cannot write to another repository, and it is set in the same environment. + - The empty `credential.helper` value is what **resets** an inherited helper list. Without it, a helper from the machine's global config is consulted first. Proven against a deliberately hostile global helper. + - `gh repo clone` leaves **no** credential in `.git/config` (it uses a temporary helper), so the clone never needed the URL rewrite in the first place. + - The push must run with the token in its environment. A `git push` that inherits the step's `GH_TOKEN` would use the wrong token, and the failure is a 403, not a prompt. + + Any script that pushes with a credential must do the same: the safe default is to have no script anywhere embed a token in a URL. ---