Refresh Linux host artifacts#125
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds ChangesLinux host artifact refresh and publish helper
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #126 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/qsl_common.sh`:
- Around line 193-208: The temporary file created with mktemp on line 193 is
placed in the system's default temporary directory, which may be on a different
filesystem than the destination. This causes the mv command on line 207 to
potentially perform a non-atomic copy+unlink operation instead of an atomic
rename, risking incomplete writes if interrupted. Modify the mktemp call to
create the temporary file in the same directory as the destination file by using
the -p flag with the directory containing the output file, ensuring the
subsequent mv operation remains atomic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 895f1234-28c3-44ba-ba5c-7addca298d43
📒 Files selected for processing (27)
AGENTS.mdCLAUDE.mdHANDOFF.mdMILESTONES.mdPROGRESS.mddocs/dpdk_research.mddocs/nic_offload_study.mdresults/crash_recovery_validation.txtresults/dpdk_environment.txtresults/false_sharing_study.txtresults/nic_offload_environment.txtresults/numa_affinity_study.txtresults/perf_report_linux.txtresults/perf_stat_linux.txtresults/socket_load_summary.txtresults/socket_profile_loopback.txtresults/socket_stress_summary.txtscripts/crash_recovery_validation.shscripts/dpdk_environment_check.shscripts/nic_offload_check.shscripts/numa_affinity_study.shscripts/perf_record.shscripts/perf_stat.shscripts/profile_gateway_io.shscripts/qsl_common.shscripts/socket_load.shscripts/socket_stress.sh
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fc8d2cec7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| link/ether 7e:c8:91:10:5e:af brd ff:ff:ff:ff:ff:ff permaddr 74:a6:cd:93:74:cc promiscuity 0 allmulti 0 minmtu 68 maxmtu 1500 netns-immutable addrgenmode none numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 gso_ipv4_max_size 65536 gro_ipv4_max_size 65536 parentbus pci parentdev 0000:01:00.0 | ||
| altname wlp1s0f0 | ||
| altname wlx74a6cd9374cc |
There was a problem hiding this comment.
When this artifact is committed or shared publicly, the raw ip -details output exposes stable Wi-Fi hardware identifiers (permaddr and the wlx... altname), which violates the repo's no-secrets/no-host-identifiers expectation for generated evidence. Please sanitize MAC addresses before publishing the NIC capability report so future refreshes do not leak device-specific identifiers.
Useful? React with 👍 / 👎.
| Linux host artifacts. `git diff --check`, Bash syntax checks, ShellCheck for the touched artifact | ||
| scripts, and host-level `make check` passed. Draft PR #125 is open for review: | ||
| <https://github.com/div0rce/quant-systems-lab/pull/125>. | ||
| - **Next action:** review draft PR #125; do not merge from automation. |
There was a problem hiding this comment.
This new next action points agents at draft PR #125, but the same file still ends with a Next action remains section telling them to wait on M49 PR #124, and HANDOFF.md still lists completed M47 as the top current priority. Because the repo tells agents to reconstruct state from these files on resume, these contradictory instructions can send the next session back to already-completed work instead of the artifact-refresh review.
Useful? React with 👍 / 👎.
Address review findings on PR #125: - qsl_publish_artifact now creates its cleaned temp file in the destination directory, so the final mv is an atomic same-filesystem rename rather than a cross-filesystem copy+unlink that could leave a partially written artifact if interrupted. - qsl_publish_artifact now redacts host MAC identifiers (link/ether, permaddr, and the wlx<mac> altname) before publishing, so generated evidence can no longer leak stable hardware identifiers (golden rule #15: no secrets in git). - Redact the Wi-Fi MAC / permaddr / altname already present in results/nic_offload_environment.txt. - Reconcile stale resume state: PROGRESS.md "Next action remains" and HANDOFF.md priority order now point at PR #125 instead of the merged M47/M49 work. The committed nic artifact was sanitized in place to remove the identifiers immediately; the Linux host artifacts should be regenerated on that host to refresh source-digest provenance and apply the new publish-time MAC sanitization.
There was a problem hiding this comment.
No application code in the PR — skipped Code Health checks.
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Point-in-time limitations audit of Quant Systems Lab for the PR #124/#125 state. Records the three PR #125 review findings (host-MAC leak, non-atomic publish, stale resume docs), their on-branch fix (perf/linux-host-artifact-refresh, commit 3731057), and step-by-step instructions for the Linux agent to finish the artifact regeneration that macOS cannot run. Host identifiers are described, not reproduced. Handoff/working doc; not intended for merge.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 373105794f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Git commit (informational): 4d8d13b | ||
| Source digest: sha256:f05d1a186fb93fdde51c8d797cc84f56567880ef15e70fb6f4e7f96f29ffee2f | ||
| Git commit (informational): fd99ad8 | ||
| Source digest: sha256:c7f574e1260e1f7bdc475423b302108e2708fe5ba8d3e115ae9d38f003a0b978 |
There was a problem hiding this comment.
The refreshed artifact records this source digest as clean provenance, but the current tree no longer hashes to it: recomputing the crash-recovery-validation digest from the committed inputs now yields sha256:380930b985462931bf26668fd2cea017698a961958df3e28b1eb9d0bba53033d, not the stored value, and the same stale-digest mismatch shows up across the refreshed result files. Because these Source digest fields are the authoritative artifact identity after the provenance migration, leaving them stale makes consumers treat evidence generated by an older generator state as if it matches this commit, especially after scripts/qsl_common.sh changed publishing behavior.
Useful? React with 👍 / 👎.
1 participant
Summary
Evidence Boundaries
nic_offload_environment.txtis a read-only capability observation forwld0Broadcom BCM4387 viabrcmfmac; no offload/RSS/timestamp/driver state was changed and no NIC latency benchmark ran.dpdk_environment.txtrecordslinux-missing-dpdk; no hugepages, device binding, or packet-path benchmark ran.perf_stat_linux.txtis partial PMU evidence only: cycles/instructions/branches/branch-misses are visible, but cache-reference/cache-miss counters are unsupported on this host.Verification
git diff --checkfind scripts -maxdepth 1 -name '*.sh' -exec bash -n {} \;shellcheck scripts/qsl_common.sh scripts/dpdk_environment_check.sh scripts/nic_offload_check.sh scripts/perf_stat.sh scripts/perf_record.sh scripts/numa_affinity_study.sh scripts/profile_gateway_io.sh scripts/socket_load.sh scripts/socket_stress.sh scripts/crash_recovery_validation.shmake checkwith host socket access: all 240 tests passedNote: a sandboxed
make checkrun failed to create the TCP loopback listener and was interrupted; the host-level rerun passed.Summary by CodeRabbit
Documentation
Chores