feat: TTD Hardening Sprint S1 — Gates and Evidence Integrity #283
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,277 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots> | ||
| name: det-gates | ||
|
|
||
| on: | ||
| pull_request: | ||
| push: | ||
| branches: [main] | ||
|
|
||
| jobs: | ||
| classify-changes: | ||
| name: classify-changes | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| run_full: ${{ steps.classify.outputs.run_full }} | ||
| run_reduced: ${{ steps.classify.outputs.run_reduced }} | ||
| run_none: ${{ steps.classify.outputs.run_none }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Detect changed files | ||
| id: changed | ||
| env: | ||
| BASE_REF: ${{ github.base_ref }} | ||
| EVENT_NAME: ${{ github.event_name }} | ||
| run: | | ||
| if [ "$EVENT_NAME" = "pull_request" ]; then | ||
| git fetch origin "$BASE_REF" --depth=1 | ||
| git diff --name-only "origin/$BASE_REF...HEAD" > changed.txt | ||
| else | ||
| git diff --name-only HEAD~1..HEAD > changed.txt || true | ||
| fi | ||
|
Comment on lines
+28
to
+34
Contributor
There was a problem hiding this comment. Push-to-main path only inspects the last commit — multi-commit merges or force pushes can slip through unclassified. Line 33: If branch protection rules prevent force pushes to 🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Acknowledged — The
Contributor
There was a problem hiding this comment.
Would you like me to open a GitHub issue to track the multi-commit merge edge case for future hardening? That way you have a backlog item if branch-protection rules ever change or if you want defense-in-depth for accidental force-pushes. ✏️ Learnings added
🧠 Learnings used |
||
| echo "Changed files:" | ||
| cat changed.txt || true | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| - name: Convert policy to JSON | ||
| run: | | ||
| yq -o=json det-policy.yaml > det-policy.json | ||
|
|
||
| - name: Classify path impact from det-policy.yaml | ||
| id: classify | ||
| run: | | ||
| node ./scripts/classify_changes.cjs det-policy.json changed.txt >> $GITHUB_OUTPUT | ||
|
|
||
|
Comment on lines
+10
to
+46
Contributor
There was a problem hiding this comment. 🛠️ Refactor suggestion | 🟠 Major No GitHub Actions default job timeout is 360 minutes. You have 6 Rust compilation jobs, a Criterion benchmark, and a WASM double-build. Any of these can hang on a linker, a flaky network fetch of crate dependencies, or an infinite-loop regression in the code under test. Without explicit timeouts, a single hung job burns 6 hours of billable CI minutes before auto-cancel, and blocks the entire evidence pipeline since Set ⏱️ Proposed fix — add timeout-minutes to each job classify-changes:
name: classify-changes
runs-on: ubuntu-latest
+ timeout-minutes: 5
outputs: determinism-linux:
name: G1 determinism (linux)
needs: classify-changes
if: needs.classify-changes.outputs.run_full == 'true'
runs-on: ubuntu-latest
+ timeout-minutes: 20
steps: determinism-macos:
name: G1 determinism (macos)
needs: classify-changes
if: needs.classify-changes.outputs.run_full == 'true'
runs-on: macos-latest
+ timeout-minutes: 20
steps: static-inspection:
name: DET-001 Static Inspection
needs: classify-changes
if: needs.classify-changes.outputs.run_full == 'true'
runs-on: ubuntu-latest
+ timeout-minutes: 10
steps: decoder-security:
name: G2 decoder security tests
needs: classify-changes
if: needs.classify-changes.outputs.run_full == 'true' || needs.classify-changes.outputs.run_reduced == 'true'
runs-on: ubuntu-latest
+ timeout-minutes: 15
steps: perf-regression:
name: G3 perf regression (criterion)
needs: classify-changes
if: needs.classify-changes.outputs.run_full == 'true'
runs-on: ubuntu-latest
+ timeout-minutes: 30
steps: build-repro:
name: G4 build reproducibility (wasm)
needs: classify-changes
if: needs.classify-changes.outputs.run_full == 'true' || needs.classify-changes.outputs.run_reduced == 'true'
runs-on: ubuntu-latest
+ timeout-minutes: 30
steps: validate-evidence:
name: Evidence schema / claim policy
+ timeout-minutes: 5
needs:🤖 Prompt for AI Agents |
||
| determinism-linux: | ||
| name: G1 determinism (linux) | ||
| needs: classify-changes | ||
| if: needs.classify-changes.outputs.run_full == 'true' | ||
| runs-on: ubuntu-latest | ||
|
Comment on lines
+47
to
+51
Contributor
There was a problem hiding this comment. 🧹 Nitpick | 🔵 Trivial No Rust/cargo caching across any of the six jobs — every PR pays full compilation cost from scratch. Six jobs ( Exception: ⚡ Example — add rust-cache to test jobs - uses: actions/checkout@v4
- name: Setup Rust
uses: dtolnay/rust-toolchain@stable
- uses: Swatinem/rust-cache@v2 # Add this after toolchain setup
- name: Run parity tests (linux)
run: ...Also applies to: 80-84, 113-117, 147-151, 167-171, 185-189 🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Acknowledged — Adding
Contributor
There was a problem hiding this comment. Rate Limit Exceeded
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Setup Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
| - name: Run parity tests (linux) | ||
| run: | | ||
| cargo test -p echo-scene-port test_float_parity_with_js -- --nocapture 2>&1 | tee det-linux.log | ||
| grep -q " 0 passed" det-linux.log && echo "FATAL: zero tests matched filter" && exit 1 || true | ||
| - name: Run DIND suite (linux) | ||
| run: | | ||
| node scripts/dind-run-suite.mjs --mode run | tee dind-linux.log | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - name: Create digest table | ||
| env: | ||
| COMMIT_SHA: ${{ github.sha }} | ||
| RUN_ID: ${{ github.run_id }} | ||
| run: | | ||
| mkdir -p artifacts | ||
| echo "target,commit,run_id,digest" > artifacts/digest-table.csv | ||
| echo "linux,${COMMIT_SHA},${RUN_ID},$(sha256sum dind-report.json | cut -d' ' -f1)" >> artifacts/digest-table.csv | ||
| - name: Upload artifacts | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: det-linux-artifacts | ||
| path: | | ||
| det-linux.log | ||
| dind-linux.log | ||
| dind-report.json | ||
| artifacts/digest-table.csv | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| determinism-macos: | ||
| name: G1 determinism (macos) | ||
| needs: classify-changes | ||
| if: needs.classify-changes.outputs.run_full == 'true' | ||
| runs-on: macos-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Setup Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
| - name: Run parity tests (macos) | ||
| run: | | ||
| cargo test -p echo-scene-port test_float_parity_with_js -- --nocapture 2>&1 | tee det-macos.log | ||
| grep -q " 0 passed" det-macos.log && echo "FATAL: zero tests matched filter" && exit 1 || true | ||
| - name: Run DIND suite (macos) | ||
| run: | | ||
| node scripts/dind-run-suite.mjs --mode run | tee dind-macos.log | ||
| - name: Create digest table | ||
| env: | ||
| COMMIT_SHA: ${{ github.sha }} | ||
| RUN_ID: ${{ github.run_id }} | ||
| run: | | ||
| mkdir -p artifacts | ||
| echo "target,commit,run_id,digest" > artifacts/digest-table.csv | ||
| echo "macos,${COMMIT_SHA},${RUN_ID},$(shasum -a 256 dind-report.json | cut -d' ' -f1)" >> artifacts/digest-table.csv | ||
| - name: Upload artifacts | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: det-macos-artifacts | ||
| path: | | ||
| det-macos.log | ||
| dind-macos.log | ||
| dind-report.json | ||
| artifacts/digest-table.csv | ||
|
|
||
| static-inspection: | ||
| name: DET-001 Static Inspection | ||
| needs: classify-changes | ||
| if: needs.classify-changes.outputs.run_full == 'true' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Install ripgrep | ||
| run: sudo apt-get update && sudo apt-get install -y ripgrep | ||
| - name: Run determinism check | ||
| id: det_check | ||
| env: | ||
| DETERMINISM_PATHS: "crates/warp-core crates/warp-geom crates/warp-wasm crates/warp-ffi crates/echo-wasm-abi crates/echo-scene-port crates/echo-scene-codec crates/echo-graph crates/echo-ttd crates/echo-dind-harness crates/echo-dind-tests crates/ttd-browser crates/ttd-protocol-rs crates/ttd-manifest" | ||
| run: | | ||
| ./scripts/ban-nondeterminism.sh | tee static-inspection.log | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - name: Create report | ||
| if: always() | ||
| env: | ||
| DET_OUTCOME: ${{ steps.det_check.outcome }} | ||
| run: | | ||
| if [ "$DET_OUTCOME" = "success" ]; then | ||
| echo '{"claim_id": "DET-001", "status": "PASSED"}' > static-inspection.json | ||
| else | ||
| echo '{"claim_id": "DET-001", "status": "FAILED"}' > static-inspection.json | ||
| fi | ||
| - name: Upload inspection artifacts | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: static-inspection | ||
| path: | | ||
| static-inspection.log | ||
| static-inspection.json | ||
|
|
||
| decoder-security: | ||
| name: G2 decoder security tests | ||
| needs: classify-changes | ||
| if: needs.classify-changes.outputs.run_full == 'true' || needs.classify-changes.outputs.run_reduced == 'true' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Setup Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
| - name: Run codec tests | ||
| run: | | ||
| cargo test -p echo-scene-codec --lib cbor::tests -- --nocapture 2>&1 | tee sec-tests.log | ||
| grep -q " 0 passed" sec-tests.log && echo "FATAL: zero tests matched filter" && exit 1 || true | ||
| - name: Upload security artifacts | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: sec-artifacts | ||
| path: | | ||
| sec-tests.log | ||
| docs/determinism/sec-claim-map.json | ||
|
|
||
| perf-regression: | ||
| name: G3 perf regression (criterion) | ||
| needs: classify-changes | ||
| if: needs.classify-changes.outputs.run_full == 'true' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Setup Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
| - name: Run benchmarks | ||
| run: | | ||
| cargo bench -p warp-benches --bench materialization_hotpath -- --output-format bencher | tee perf.log | ||
| - name: Upload perf artifacts | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: perf-artifacts | ||
| path: perf.log | ||
|
Comment on lines
173
to
190
Contributor
There was a problem hiding this comment. G3 "perf regression" doesn't detect regressions — there is no baseline comparison. The job runs Criterion benchmarks, captures output, and uploads it. That's a "benchmarks compile and run" gate. The name says "perf regression" but there is no historical baseline, no threshold, no comparison, and no failure criterion tied to performance. A 10x regression in At minimum, either:
If option 1 for now, document that regression detection is a future enhancement. 🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Acknowledged — G3 currently gates on benchmark compilation and execution, not regression detection. True regression detection requires baseline storage (e.g., criterion's baseline comparison or GitHub Action benchmark caching). This is a known limitation and a future enhancement.
Contributor
There was a problem hiding this comment. Rate Limit Exceeded
|
||
|
|
||
| build-repro: | ||
| name: G4 build reproducibility (wasm) | ||
| needs: classify-changes | ||
| if: needs.classify-changes.outputs.run_full == 'true' || needs.classify-changes.outputs.run_reduced == 'true' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Setup Rust (Global) | ||
| uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| targets: wasm32-unknown-unknown | ||
| - name: Checkout Build 1 | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| path: build1 | ||
| - name: Build 1 | ||
| run: | | ||
| cd build1 | ||
| rustup target add wasm32-unknown-unknown | ||
| cargo build --release --target wasm32-unknown-unknown -p ttd-browser | ||
| sha256sum target/wasm32-unknown-unknown/release/ttd_browser.wasm > ../hash1.txt | ||
| cp target/wasm32-unknown-unknown/release/ttd_browser.wasm ../build1.wasm | ||
| - name: Checkout Build 2 | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| path: build2 | ||
| - name: Build 2 | ||
| run: | | ||
| cd build2 | ||
| rustup target add wasm32-unknown-unknown | ||
| cargo build --release --target wasm32-unknown-unknown -p ttd-browser | ||
| sha256sum target/wasm32-unknown-unknown/release/ttd_browser.wasm > ../hash2.txt | ||
| cp target/wasm32-unknown-unknown/release/ttd_browser.wasm ../build2.wasm | ||
|
Comment on lines
+206
to
+223
Contributor
There was a problem hiding this comment. 🧹 Nitpick | 🔵 Trivial Redundant Lines 202 and 213 both run 🧹 Proposed cleanup - name: Build 1
run: |
cd build1
- rustup target add wasm32-unknown-unknown
cargo build --release --target wasm32-unknown-unknown -p ttd-browser
sha256sum target/wasm32-unknown-unknown/release/ttd_browser.wasm > ../hash1.txt
cp target/wasm32-unknown-unknown/release/ttd_browser.wasm ../build1.wasm
- name: Checkout Build 2
uses: actions/checkout@v4
with:
path: build2
- name: Build 2
run: |
cd build2
- rustup target add wasm32-unknown-unknown
cargo build --release --target wasm32-unknown-unknown -p ttd-browser🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Acknowledged — The
Contributor
There was a problem hiding this comment. Rate Limit Exceeded
|
||
| - name: Compare hashes | ||
| run: | | ||
| diff hash1.txt hash2.txt || (echo "Reproducibility failure: Hashes differ!" && exit 1) | ||
| echo "Hashes match: $(cat hash1.txt)" | ||
| - name: Upload build artifacts | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: build-repro-artifacts | ||
| path: | | ||
| hash1.txt | ||
| hash2.txt | ||
| build1.wasm | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| build2.wasm | ||
|
|
||
| validate-evidence: | ||
| name: Evidence schema / claim policy | ||
| needs: | ||
| - classify-changes | ||
| - determinism-linux | ||
| - determinism-macos | ||
| - static-inspection | ||
| - decoder-security | ||
| - perf-regression | ||
| - build-repro | ||
| if: always() && needs.classify-changes.outputs.run_none != 'true' | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Download all artifacts | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| path: gathered-artifacts | ||
| - name: Verify artifact presence | ||
| env: | ||
| RUN_FULL: ${{ needs.classify-changes.outputs.run_full }} | ||
| run: | | ||
| ls -R gathered-artifacts | ||
| # Always required (run on both full and reduced) | ||
| [ -d gathered-artifacts/sec-artifacts ] || (echo "Missing sec-artifacts" && exit 1) | ||
| [ -d gathered-artifacts/build-repro-artifacts ] || (echo "Missing build-repro-artifacts" && exit 1) | ||
| # Only required when run_full (these jobs are skipped for run_reduced) | ||
| if [ "$RUN_FULL" = "true" ]; then | ||
| [ -d gathered-artifacts/det-linux-artifacts ] || (echo "Missing det-linux-artifacts" && exit 1) | ||
| [ -d gathered-artifacts/det-macos-artifacts ] || (echo "Missing det-macos-artifacts" && exit 1) | ||
| [ -d gathered-artifacts/perf-artifacts ] || (echo "Missing perf-artifacts" && exit 1) | ||
| [ -d gathered-artifacts/static-inspection ] || (echo "Missing static-inspection" && exit 1) | ||
| fi | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+239
to
+271
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: cd /tmp && find . -name "validate_claims.cjs" 2>/dev/null | head -5Repository: flyingrobots/echo Length of output: 43 🏁 Script executed: git ls-files | grep -E "(validate_claims|det-gates)" | head -20Repository: flyingrobots/echo Length of output: 120 🏁 Script executed: cat scripts/validate_claims.cjsRepository: flyingrobots/echo Length of output: 2442 Content validation for FAILED evidence is missing — validate_claims.cjs only checks VERIFIED claims. The The script at for (const claim of data.claims) {
if (claim.status === 'VERIFIED') {
// validate required pointers...
}
}It exclusively validates claims marked Add an explicit content check in the 🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. By design —
Contributor
There was a problem hiding this comment. Rate Limit Exceeded
|
||
| - name: Generate evidence pack | ||
| run: | | ||
| ./scripts/generate_evidence.cjs gathered-artifacts | ||
| - name: Validate evidence pointers | ||
| run: | | ||
| ./scripts/validate_claims.cjs gathered-artifacts/evidence.json | ||
|
Comment on lines
+272
to
+277
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# First, get the full workflow file to check invocation patterns
cat -n .github/workflows/det-gates.yml | head -280 | tail -50Repository: flyingrobots/echo Length of output: 2511 🏁 Script executed: #!/bin/bash
# Also check around line 45 to verify the classify step
cat -n .github/workflows/det-gates.yml | sed -n '40,50p'Repository: flyingrobots/echo Length of output: 518 🏁 Script executed: #!/bin/bash
# Check execute permissions on all CJS scripts in scripts/
fd -e cjs . scripts/ --exec stat -c '%A %n' {} \;Repository: flyingrobots/echo Length of output: 204 🏁 Script executed: #!/bin/bash
# Check git filemode tracking for CJS scripts
fd -e cjs . scripts/ --exec git ls-files -s {} \;Repository: flyingrobots/echo Length of output: 379 Inconsistent script invocation patterns — lines 264 & 267 must use Lines 264 and 267 invoke Proposed fix - name: Generate evidence pack
run: |
- ./scripts/generate_evidence.cjs gathered-artifacts
+ node ./scripts/generate_evidence.cjs gathered-artifacts
- name: Validate evidence pointers
run: |
- ./scripts/validate_claims.cjs gathered-artifacts/evidence.json
+ node ./scripts/validate_claims.cjs gathered-artifacts/evidence.json🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Acknowledged — will review the invocation patterns in a follow-up pass.
Contributor
There was a problem hiding this comment. Rate Limit Exceeded
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
No
concurrencygroup — parallel runs from rapid pushes can produce interleaved artifacts and confusing evidence chains.Rapid pushes to the same branch or multiple PRs from the same head can trigger overlapping workflow runs. Without a
concurrencykey withcancel-in-progress, stale runs waste CI minutes and their artifacts can intermingle with current runs in artifact storage.♻️ Proposed addition
on: pull_request: push: branches: [main] +concurrency: + group: det-gates-${{ github.ref }} + cancel-in-progress: true + permissions: contents: read actions: read📝 Committable suggestion
🤖 Prompt for AI Agents
Missing
permissions:block — a security hardening sprint that doesn't lock down its own workflow token is ironic.This workflow has no
permissions:key. On public repos the defaults are read-only, but on forks or private repos theGITHUB_TOKENmay carrywriteon contents, packages, issues, etc. For a PR whose title is literally "Hardening Sprint", explicitly declaring the minimum required permissions is table stakes. You only needcontents: readand potentiallyactions: readfor artifact downloads.🔒 Proposed fix — least-privilege permissions
name: det-gates on: pull_request: push: branches: [main] +permissions: + contents: read + actions: read + jobs:🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged — Valid improvement for CI efficiency. Adding a concurrency group with cancel-in-progress would prevent artifact interleaving from rapid pushes. Deferring to a follow-up CI hardening pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flyingrobotsUnderstood — you're deferring theconcurrencyblock.However: This PR is titled "TTD Hardening Sprint S1 — Gates and Evidence Integrity." Deferring a CI robustness fix from the hardening sprint feels like shipping the fire extinguisher without the pin. The artifact interleaving risk isn't theoretical — rapid pushes will race, and your evidence validation in
validate-evidencewill ingest artifacts from overlapping runs. That's a direct threat to the integrity claims this sprint is meant to harden.Recommendation: Either:
concurrencyblock now (2 lines, zero risk), orWhich do you prefer?
🧠 Learnings used