gps3: surface-scan resolution, tmux runbook, and the Bernese validator C2 fix - #64
Merged
Conversation
Session log §13-14, plus the tooling and doc corrections behind them. §13.2 closes the §12.4 gap. The premise was too narrow: it assumed surface scanning could only come from the PERC, so an iDRAC with no IP made the question unanswerable. It comes instead from BMS, the drive firmware's own Background Media Scan, which runs below the controller and is readable via the megaraid pass-through. All 16 members sweep their full surface roughly daily (SWEEPS 1.01x, ~425 scans over 10,145 power-on hours), with zero uncorrected errors and ~112 sectors repaired in place while parity was still intact. SMART long tests are therefore rejected as duplicative and the §9 smartd config stands unchanged. Member 6 was investigated as a probable early failure at 671 recovered sectors, seven times its nearest peer, and cleared. These are 512e drives: one 4K physical sector logs as eight 512-byte entries, so 671/8 ~= 84 — matching that drive's own corrected-error counter exactly. Defects span hours 121 to 10,127 at a flat rate, nothing reassigned. The instructive part is that member 0, with zero entries, carries 228 corrected read errors and 3,929 correction invocations: ranking drives by defect count alone picks the wrong suspect. Bernese: USER.CPU maxjobs was 2 — the T420's value carried across, so the R740 was using 2 of its 12 cores against the known 502 GPSCLU_P bottleneck. Now 11, set through cpu_config.compute_maxjobs() rather than by hand. The readiness doc's "24 physical cores" was the logical count; the box is a 12-core Xeon Silver 4214R, and since maxjobs tracks physical cores for FPU-bound sub-solves, 24 would have oversubscribed by 2x. AVX-512 is present, so the objcopy ISA patch that doc flagged as uncertain is not needed. New P0 blocker C2: validate_rinex_headers() finds zero files in the real DATAPOOL. _is_rinex_obs() matches on path.suffix, but every file there is gzipped, and decompressed they are Hatanaka .26d or .crx — none accepted. It failed loudly only because require_stations=True was passed; under the default it returns a PASSING report having inspected nothing, which is the vacuous pass its own docstring warns about. All 128 tests pass, because the fixtures use uncompressed names. Tooling: patrol_check.sh moved into the repo with both defects fixed (the error-counter columns were transposed, reporting 0 TB read and 1,025,339 uncorrected errors; and the recovered column now reports physical sectors). Root-requiring run-scripts now live in scripts/sudo/ with logs gitignored and the scripts committed, so sudo steps need no copy-paste between terminals. Adds docs/gps3_tmux_claude_runbook.md: how to run Claude Code under tmux on gps3, written for someone who has never used tmux. Start tmux first — a running process cannot be moved into it later — and note that a reboot destroys every session while cron and smartd carry on regardless.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds RAID diagnostics, GPS3 operational documentation, compressed and Hatanaka RINEX validation, GPSUSER provisioning, panel safeguards, and station campaign registration tooling. ChangesGPS3 operations
Bernese readiness
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RINEXScanner
participant Validator
participant Decompressor
participant CampaignFiles
RINEXScanner->>Validator: discover and parse RINEX headers
Validator->>Decompressor: open plain or compressed observation data
Decompressor-->>Validator: return station metadata
Validator->>CampaignFiles: plan or apply station rows
CampaignFiles-->>Validator: report updates and backups
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@docs/gps3_tmux_claude_runbook.md`:
- Around line 134-149: Update the documented sudo execution workflow to require
a human review gate before running any script: inspect the exact script and git
diff, verify the expected commit and file ownership/permissions, and record the
commit or checksum. Require approved operations to use root-owned,
non-user-writable wrappers where possible, then execute only the verified script
path with sudo.
- Around line 165-174: Update the quick-reference fenced block containing the
tmux commands and prose to specify the text language, using a text fence while
preserving all block contents.
- Line 105: Update the rsync command in the runbook to use a shell-safe source
placeholder and quote both the source and destination paths, preserving the
existing archive destination.
- Around line 57-60: Add an approved-data rule to the runbook before the GPS3
execution steps, explicitly prohibiting credentials, tokens, personal data, and
restricted operational data from sudo output sent to Claude unless the approved
account and retention policy permit them.
- Around line 103-116: Update the archive transfer command in the tmux runbook
to include rsync’s --partial option, allowing interrupted file transfers to
resume from received data. Leave the surrounding tmux instructions and
independent census completeness check unchanged.
In `@docs/gps3-sessions/SESSION_LOG_20260729_storage.md`:
- Around line 715-718: Update the PR reference in the paragraph around “Rule 2”
so the leading hash in “#57” is escaped or replaced with “PR 57”, preventing
Markdown from interpreting it as a heading.
In `@docs/project_documentation/bernese_orchestrator_r740_readiness.md`:
- Around line 104-120: Make the validation flow fail closed when no observation
files are recognized: require at least one recognized file before returning a
passing report, regardless of require_stations. Update the relevant validator
logic and add a regression test covering the default require_stations=False
setting, while preserving existing station-specific behavior when files are
present.
- Around line 117-120: Update the documented processing requirements for `.Z`
inputs to specify a compatible Unix-compress decoder before header parsing,
alongside the existing `gzip` and `CRX2RNX` steps. Extend the end-to-end fixture
coverage for the real DATAPOOL naming scheme to include a `.Z` file and verify
it is accepted through the `RinexQC.run_qc` path.
In `@scripts/patrol_check.sh`:
- Around line 78-82: Update the recovery parsing around the rec count and its
reporting at Lines 127-128 so only entries with reassign_status “Recovered via
rewrite in-place” are counted as repaired. Track “Reassigned” and “Reassign
failed” entries separately, and treat those statuses as escalation conditions
rather than including them in the repaired total.
- Around line 99-115: Update scripts/patrol_check.sh lines 99-115 to track
required health fields for every one of the 16 members, treating unavailable
values as failures and exiting non-zero instead of returning success with “?” or
“-” placeholders. Update scripts/sudo/verify_patrol_check.sh lines 41-52 to
parse each captured member record and fail when UNCORR is non-zero, any required
value is missing, or SWEEPS is unavailable or non-numeric; preserve successful
verification only when all members pass these checks.
In `@scripts/sudo/inspect_member6.sh`:
- Around line 88-93: Replace the predictable temporary path used by the smartctl
pipeline in inspect_member6.sh with a securely created private temporary file,
using its generated path for head, tail, and cleanup. Ensure the file is created
with restrictive permissions and remains safely removed after processing.
In `@scripts/sudo/verify_patrol_check.sh`:
- Line 25: Update the TARGET initialization in verify_patrol_check.sh to resolve
scripts/patrol_check.sh relative to the verification script by default, rather
than using the absolute /home/gps3 path. Preserve support for a deployed or
alternate target only when supplied explicitly as the script’s argument.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 47a02212-ed75-4288-9ff6-99928c6d9577
📒 Files selected for processing (7)
.gitignoredocs/gps3-sessions/SESSION_LOG_20260729_storage.mddocs/gps3_tmux_claude_runbook.mddocs/project_documentation/bernese_orchestrator_r740_readiness.mdscripts/patrol_check.shscripts/sudo/inspect_member6.shscripts/sudo/verify_patrol_check.sh
Comment on lines
+57
to
+60
| Two independent things persist, and it helps to keep them straight: | ||
|
|
||
| - **The tmux session** lives on gps3 and holds the *running process*. | ||
| - **The conversation** lives on Anthropic's servers and holds the *history*. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tracked files matching runbook =="
git ls-files | rg '(^|/)gps3_tmux_claude_runbook\.md$|sudo/logs'
echo
echo "== docs/gps3_tmux_claude_runbook.md outline/context around cited lines =="
if [ -f docs/gps3_tmux_claude_runbook.md ]; then
wc -l docs/gps3_tmux_claude_runbook.md
sed -n '45,75p;125,155p' docs/gps3_tmux_claude_runbook.md | cat -n
fi
echo
echo "== references to Claude Code, Claude, github.com/anthropics/claude-code, scripts/sudo =="
rg -n "Claude Code|Claude|anthropic|github\.com/anthropics/claude-code|scripts/sudo|sudo/logs|Claude Code session|session" docs/gps3_tmux_claude_runbook.md .gitignore scripts 2>/dev/null || true
echo
echo "== files under docs and gitignore mentioning Claude/sudo/logs =="
git ls-files docs scripts 'scripts/sudo/*' 2>/dev/null | sed -n '1,120p'Repository: alfieprojectsdev/movefaults
Length of output: 12498
Define which sudo output Claude may receive.
This runbook says script output is sent to scripts/sudo/logs/ and then read by Claude. Add an approved-data rule before running this on GPS3: prohibit credentials, tokens, personal data, and restricted operational data unless the approved account and retention policy allow them.
🤖 Prompt for 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.
In `@docs/gps3_tmux_claude_runbook.md` around lines 57 - 60, Add an approved-data
rule to the runbook before the GPS3 execution steps, explicitly prohibiting
credentials, tokens, personal data, and restricted operational data from sudo
output sent to Claude unless the approved account and retention policy permit
them.
Comment on lines
+103
to
+116
| ```bash | ||
| tmux new -s archive | ||
| rsync -av --info=progress2 <source>/ /srv/gnss-archive/legacy/ | ||
| # Ctrl-b d to detach; check back whenever | ||
| ``` | ||
|
|
||
| Then reattach with `tmux attach -t archive` to see progress. | ||
|
|
||
| Run it inside tmux **on both ends** if the transfer is initiated from another | ||
| machine — a dropped SSH session kills the *client* side just as dead. | ||
|
|
||
| **Do not trust `rsync`'s exit code as proof the copy is complete.** rsync exits | ||
| 0 having skipped files it could not read, and exits 23 on a run that copied | ||
| 99.99% successfully. Verify with an independent census afterwards: |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Target file context:\n'
if [ -f docs/gps3_tmux_claude_runbook.md ]; then
nl -ba docs/gps3_tmux_claude_runbook.md | sed -n '90,125p'
else
printf 'docs/gps3_tmux_claude_runbook.md not found\n'
fi
printf '\nSearch for rsync options in repo:\n'
rg -n "rsync|partial|exit 0|exit 23|incomplete|census" .
printf '\nBehavioral check: deterministic man-page/dataset-free reading of installed rsync if available:\n'
if command -v rsync >/dev/null; then
printf 'rsync version: '; rsync --version | head -n1
printf '\nRelevant man-page sections:\n'
man rsync 2>/dev/null | sed -n '/^ESCAPE CODES/,/^DESCRIPTION/p' | sed -n '1,160p'
printf '\nHelp snippets:\n'
rsync -h 2>&1 | sed -n '/partial/,+5p'
else
printf 'rsync not installed\n'
fiRepository: alfieprojectsdev/movefaults
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -e
printf 'Target file context:\n'
if [ -f docs/gps3_tmux_claude_runbook.md ]; then
awk '{printf "%6d\t%s\n", NR, $0}' docs/gps3_tmux_claude_runbook.md | sed -n '90,125p'
else
printf 'docs/gps3_tmux_claude_runbook.md not found\n'
fi
printf '\nSearch for rsync-related guidance in repo:\n'
rg -n "rsync|partial|exit 0|exit 23|incomplete|census" . || true
printf '\nDocumentation/resource references in repository:\n'
git ls-files | rg '(doc|man|readme|runbook|faq)' || trueRepository: alfieprojectsdev/movefaults
Length of output: 50384
🌐 Web query:
rsync man page partial exit code 23 partial transfer --partial behavior
💡 Result:
In rsync, exit code 23 indicates a partial transfer due to an error [1][2]. This is a general error code used when one or more files or attributes could not be transferred, and no other specific exit code applies [3][4]. Common causes include files vanishing on the sender side during the transfer, permission issues, or other interruptions that prevent the completion of specific file copies [3][4]. Regarding the --partial behavior: 1. Purpose: The --partial option instructs rsync to keep partially transferred files instead of deleting them when a transfer is interrupted [5][6]. This allows a subsequent rsync run to potentially resume the transfer of those files from where they left off [7][8]. 2. Default Behavior: By default, if a transfer is interrupted, rsync deletes the partial file on the receiving side to avoid leaving incomplete or corrupted data [7][9]. 3. Interaction with --inplace: When using the --inplace option, rsync updates the destination file directly [10][8]. This implies the --partial behavior, meaning that if the transfer is interrupted, the partial data remains in the destination file [7][10]. Use caution with --inplace, as it may leave files in a partially updated state that could be problematic for other processes accessing those files [10][8][11]. 4. Options: You can also use --partial-dir=DIR to specify a directory where partially transferred files should be stored, rather than leaving them in the destination location [5][6]. Note that exit code 23 is a signal that some files or attributes failed to transfer, even if other parts of the operation succeeded [4][2]. It is often necessary to check the standard error (stderr) output from the rsync command to identify the specific cause of the failure [3][4].
Citations:
- 1: https://github.com/RsyncProject/rsync/blob/master/errcode.h
- 2: https://stackoverflow.com/questions/20737204/comprehensive-list-of-rsync-error-codes
- 3: https://lists.samba.org/archive/rsync/2023-December/033086.html
- 4: rsync backup returns error 23 - But files are copied successfully. RsyncProject/rsync#458
- 5: https://www.samba.org/ftp/rsync/rsync.html
- 6: https://man7.org/linux/man-pages/man1/rsync.1.html
- 7: https://github.com/RsyncProject/rsync/blob/b905ab23af2d71363271e99e446e8fe0bfc77f7f/rsync.1.md
- 8: https://github.com/RsyncProject/rsync/blob/d4c4f6754eff0d8ea6fdb327abf5c874bfccb8dd/rsync.1.md
- 9: https://rsync.samba.org/ftp/rsync/rsync.1
- 10: https://github.com/RsyncProject/rsync/blob/62bb9bba022ce6a29f8c92307d5569c338b2f711/rsync.1.md
- 11: https://man.archlinux.org/man/extra/rsync/rsync.1.en
Make the archive transfer resumable.
rsync -av removes a partially copied file when the transfer stops. Add --partial so interrupted runs can continue from the received data. Keep the independent census as the final completeness check.
Proposed fix
-rsync -av --info=progress2 <source>/ /srv/gnss-archive/legacy/
+rsync -av --partial --info=progress2 "/path/to/source/" "/srv/gnss-archive/legacy/"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ```bash | |
| tmux new -s archive | |
| rsync -av --info=progress2 <source>/ /srv/gnss-archive/legacy/ | |
| # Ctrl-b d to detach; check back whenever | |
| ``` | |
| Then reattach with `tmux attach -t archive` to see progress. | |
| Run it inside tmux **on both ends** if the transfer is initiated from another | |
| machine — a dropped SSH session kills the *client* side just as dead. | |
| **Do not trust `rsync`'s exit code as proof the copy is complete.** rsync exits | |
| 0 having skipped files it could not read, and exits 23 on a run that copied | |
| 99.99% successfully. Verify with an independent census afterwards: |
🧰 Tools
🪛 LanguageTool
[locale-violation] ~116-~116: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...ully. Verify with an independent census afterwards: ```bash sudo -u gps3 /srv/gnss-archiv...
(AFTERWARDS_US)
🤖 Prompt for 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.
In `@docs/gps3_tmux_claude_runbook.md` around lines 103 - 116, Update the archive
transfer command in the tmux runbook to include rsync’s --partial option,
allowing interrupted file transfers to resume from received data. Leave the
surrounding tmux instructions and independent census completeness check
unchanged.
|
|
||
| ```bash | ||
| tmux new -s archive | ||
| rsync -av --info=progress2 <source>/ /srv/gnss-archive/legacy/ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a shell-safe source placeholder.
<source>/ is parsed as shell redirection syntax when copied as written. Use a valid placeholder path and quote both paths.
Proposed fix
-rsync -av --info=progress2 <source>/ /srv/gnss-archive/legacy/
+rsync -av --info=progress2 "/path/to/source/" "/srv/gnss-archive/legacy/"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| rsync -av --info=progress2 <source>/ /srv/gnss-archive/legacy/ | |
| rsync -av --info=progress2 "/path/to/source/" "/srv/gnss-archive/legacy/" |
🤖 Prompt for 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.
In `@docs/gps3_tmux_claude_runbook.md` at line 105, Update the rsync command in
the runbook to use a shell-safe source placeholder and quote both the source and
destination paths, preserving the existing archive destination.
Comment on lines
+134
to
+149
| 1. Keep a **second terminal** SSH'd into gps3 (its own tmux session is fine). | ||
| 2. Claude writes the command as a script under | ||
| `/home/gps3/repos/movefaults_clean/scripts/sudo/` and gives you the | ||
| **absolute path only**. | ||
| 3. You run that path with `sudo` in your terminal. | ||
| 4. The script `tee`s its output to `scripts/sudo/logs/`, so you watch it live | ||
| *and* Claude can read the result. | ||
|
|
||
| ```bash | ||
| sudo /home/gps3/repos/movefaults_clean/scripts/sudo/<script>.sh | ||
| ``` | ||
|
|
||
| Nothing to transcribe between windows, no shell quoting to survive a | ||
| copy-paste, and no swallowed output. `scripts/sudo/logs/` is gitignored; **the | ||
| scripts themselves are committed**, because they are the record of what was | ||
| actually run on this machine. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Add a human review gate before root execution.
The workflow says Claude writes the script, provides only an absolute path, and the operator then runs it with sudo. This can execute unreviewed or modified working-tree content as root. Require review of the exact script and git diff, verification of the expected commit and ownership, and recording of the commit or checksum before execution. Prefer root-owned, non-user-writable wrappers for allowlisted operations.
🤖 Prompt for 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.
In `@docs/gps3_tmux_claude_runbook.md` around lines 134 - 149, Update the
documented sudo execution workflow to require a human review gate before running
any script: inspect the exact script and git diff, verify the expected commit
and file ownership/permissions, and record the commit or checksum. Require
approved operations to use root-owned, non-user-writable wrappers where
possible, then execute only the verified script path with sudo.
Comment on lines
+165
to
+174
| ``` | ||
| tmux new -A -s claude start or reattach (the safe default) | ||
| Ctrl-b then d detach, leaving everything running | ||
| tmux ls what sessions exist | ||
| tmux attach -t <name> reattach to one | ||
| claude --resume pick up a previous conversation | ||
|
|
||
| Start tmux BEFORE the work. A running process cannot be moved in later. | ||
| A reboot destroys every tmux session. Plan around it. | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the quick-reference fence.
The fence at Line 165 has no language and triggers MD040. Use text because the block contains commands and prose.
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| tmux new -A -s claude start or reattach (the safe default) | |
| Ctrl-b then d detach, leaving everything running | |
| tmux ls what sessions exist | |
| tmux attach -t <name> reattach to one | |
| claude --resume pick up a previous conversation | |
| Start tmux BEFORE the work. A running process cannot be moved in later. | |
| A reboot destroys every tmux session. Plan around it. | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 165-165: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for 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.
In `@docs/gps3_tmux_claude_runbook.md` around lines 165 - 174, Update the
quick-reference fenced block containing the tmux commands and prose to specify
the text language, using a text fence while preserving all block contents.
Source: Linters/SAST tools
Comment on lines
+117
to
+120
| > **Fix:** teach `_is_rinex_obs()` to strip a `.gz`/`.Z` suffix before matching, and | ||
| > accept Hatanaka `.<yy>d` and `.crx`. Header parsing must then decompress | ||
| > (`gzip` + `CRX2RNX`) or read headers without full decompression. Add fixtures in | ||
| > the real DATAPOOL naming scheme so the suite can see this class of failure. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Specify decoding for .Z inputs.
The requirement includes .Z, but the processing path names only gzip and CRX2RNX. A .Z file uses Unix-compress encoding and needs a compatible decoder. The existing RinexQC.run_qc wrapper also has no decompression layer. Specify the .Z decoding step and add an end-to-end fixture; otherwise .Z inputs remain unsupported.
🤖 Prompt for 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.
In `@docs/project_documentation/bernese_orchestrator_r740_readiness.md` around
lines 117 - 120, Update the documented processing requirements for `.Z` inputs
to specify a compatible Unix-compress decoder before header parsing, alongside
the existing `gzip` and `CRX2RNX` steps. Extend the end-to-end fixture coverage
for the real DATAPOOL naming scheme to include a `.Z` file and verify it is
accepted through the `RinexQC.run_qc` path.
Comment on lines
+78
to
+82
| # Rows of the recovered-sector table, e.g. | ||
| # 1 7725:11 0000000000b57030 [1,18,7] Recovered via rewrite in-place | ||
| # Each is a latent bad sector BMS found and repaired while the array still | ||
| # had full redundancy — i.e. the failure that kills a rebuild, defused. | ||
| rec=$(printf '%s\n' "$bg" | grep -cE '\[[0-9]+,[0-9]+,[0-9]+\]') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not classify every BMS log entry as repaired.
Lines 78-82 count entries without checking reassign_status. An entry such as Reassigned or Reassign failed will be included in the “repaired” total at Lines 127-128. This can report a failing member as repaired.
Count rewrite-in-place entries separately. Report other statuses as escalation conditions.
Also applies to: 127-128
🤖 Prompt for 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.
In `@scripts/patrol_check.sh` around lines 78 - 82, Update the recovery parsing
around the rec count and its reporting at Lines 127-128 so only entries with
reassign_status “Recovered via rewrite in-place” are counted as repaired. Track
“Reassigned” and “Reassign failed” entries separately, and treat those statuses
as escalation conditions rather than including them in the repaired total.
Comment on lines
+99
to
+115
| [ -z "$rgb" ] && rgb="?"; [ -z "$unc" ] && unc="?" | ||
| [ -z "$scans" ] && scans="?"; [ -z "$rec" ] && rec=0 | ||
| [ -z "$status" ] && status="(no background scan log)" | ||
|
|
||
| rtb=$(awk -v g="$rgb" 'BEGIN{ if (g+0==g) printf "%.2f", g/1000; else print "?" }') | ||
|
|
||
| # THE CROSS-CHECK, and the reason this script can conclude anything at all. | ||
| # Scan count and bytes-read are two counters the firmware maintains for | ||
| # different purposes. If (scans x capacity) lands near the lifetime read | ||
| # total, the scans really are reading the whole surface — verified by two | ||
| # independent paths rather than by trusting one number's label. | ||
| if [ "$rgb" != "?" ] && [ "$scans" != "?" ] && [ -n "$capb" ]; then | ||
| sweeps=$(awk -v g="$rgb" -v s="$scans" -v c="$capb" \ | ||
| 'BEGIN{ if (s>0 && c>0) printf "%.2fx", (g*1e9)/(s*c); else print "-" }') | ||
| else | ||
| sweeps="-" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail the diagnostic workflow when required health evidence is missing or unsafe.
The workflow converts unavailable values to ? or -, then returns success. The verifier only prints manual checks and propagates that success. A failed SMART query, non-zero UNCORR, or unavailable SWEEPS can therefore produce a successful verification log.
scripts/patrol_check.sh#L99-L115: track required fields for all 16 members and exit non-zero if any field is unavailable.scripts/sudo/verify_patrol_check.sh#L41-L52: parse the captured log and fail if a member has non-zeroUNCORR, missing values, or a non-numeric or unavailableSWEEPSresult.
📍 Affects 2 files
scripts/patrol_check.sh#L99-L115(this comment)scripts/sudo/verify_patrol_check.sh#L41-L52
🤖 Prompt for 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.
In `@scripts/patrol_check.sh` around lines 99 - 115, Update
scripts/patrol_check.sh lines 99-115 to track required health fields for every
one of the 16 members, treating unavailable values as failures and exiting
non-zero instead of returning success with “?” or “-” placeholders. Update
scripts/sudo/verify_patrol_check.sh lines 41-52 to parse each captured member
record and fail when UNCORR is non-zero, any required value is missing, or
SWEEPS is unavailable or non-numeric; preserve successful verification only when
all members pass these checks.
alfieprojectsdev
changed the title
validate_rinex_headers() found zero RINEX in the gps3 archive. _is_rinex_obs() matched on path.suffix, but all 3,010 archive files there are .gz and 20 are .Z, so suffix was always the compression extension. Hatanaka was an independent second miss: .26d and .crx were absent from the accepted set, so even decompressed most PAGENET files would still have been skipped. The consequence was worse than skipped files. An empty scan is only an error when require_stations=True; under the default the function returned a PASSING report having examined nothing — approving every session while inspecting none of it, first symptom arriving much later as an RXOBV3 hard abort mid-BPE. That is the vacuous pass its own docstring warns about, reached through an unanticipated door. All 128 tests passed throughout, because the fixtures used uncompressed .YYo/.rnx names that do not occur in the data. No Hatanaka decoding was needed. CRINEX stores the original RINEX header verbatim after two CRINEX VERS/PROG lines and compacts only the records below END OF HEADER, which this validator never reads. crx2rnx never runs; decompression alone suffices. Compression needed two paths. IGS convention is .Z (UNIX compress/LZW) and Python's gzip cannot read it — BadGzipFile, magic 1f 9d against gzip's 1f 8b. Python gzip handles .gz, keeping the 3,010-file path subprocess-free; GNU gzip -dc handles the 20 .Z files. No new dependency. Suffix stripping loops right-to-left because every real name stacks two extensions and both orders occur. Two further defects surfaced only once the scan returned anything: Descriptive marker names shadowed the station code. PAGENET CORS write MARKER NAME "BOGO CITY" with the code in MARKER NUMBER "PBOG"; IGS fiducials do the reverse. Taking MARKER NAME[:4] gave BOGO, absent from PGN.STA, failing 9 of 72 stations on data that processes correctly. _resolve_station_code() prefers a bare 4-char MARKER NUMBER, then a bare 4-char MARKER NAME, then the filename. rglob descended into .excluded_plg2/, where the two intermittent PLG2 files were hand-quarantined during the training week, reporting PLG2 missing on exactly DOY 086 and 088 — independently reproducing the readiness doc's empirical finding. RNX_COP globs without recursing, so those files can never be staged; a validator must model what will actually be processed. Dot-directories are now skipped. PLG2 remains genuinely absent from PGN.STA — the quarantine is a workaround, not a fix. Tests now span the production filename space, which is why the previous 128 could not see any of this: parametrised recognition across every real encoding plus negative cases, round-trip reads for plain/.gz/.Z, a from-scratch LZW encoder so .Z is testable without an external compressor (nothing on stock Ubuntu can write .Z), an assertion that SIGPIPE from the deliberately cut-off gzip is not treated as failure, and an integration test against the real DATAPOOL that skips off-host. All seven PAGENET sessions (DOY 084-090) now validate clean. 179 passed.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/gps3-sessions/SESSION_LOG_20260729_storage.md (3)
725-756: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not treat census errors as an empty destination.
census()inscripts/gps3_gpsdata_migrate.shredirects allfinderrors to/dev/nulland does not check thefindstatus. A missing or unreadable path can therefore produce0 0 0, causing the documented empty-destination check to pass falsely. Make each census operation fail on any filesystem error before recording transfer state.🤖 Prompt for 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. In `@docs/gps3-sessions/SESSION_LOG_20260729_storage.md` around lines 725 - 756, Update census() in scripts/gps3_gpsdata_migrate.sh so every find operation preserves and checks its exit status instead of redirecting errors to /dev/null. Make any missing or unreadable path cause the census to fail before emitting 0 counts or recording transfer state, while preserving normal empty-directory results.
489-538: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when append-only settings are missing.
The documented guarantee depends on
core.logAllRefUpdates=trueandgc.pruneExpire=never.scripts/gnss_mirror_update.shcomments that these settings are configured and verified, but it does not check either value beforegit remote update. A fresh or changed mirror can therefore complete cron runs without preserving force-pushed history. Validate both settings on every run, or make provisioning an enforced prerequisite.🤖 Prompt for 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. In `@docs/gps3-sessions/SESSION_LOG_20260729_storage.md` around lines 489 - 538, Update scripts/gnss_mirror_update.sh to validate core.logAllRefUpdates=true and gc.pruneExpire=never before running git remote update. If either setting is missing or differs, fail closed with a clear error and do not perform the mirror update; retain the existing mount and noninteractive safeguards.
627-686: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake patrol verification enforce the health criteria.
scripts/sudo/verify_patrol_check.shonly tees output, prints manual checks, and returnspatrol_check.sh's exit status.scripts/patrol_check.shdoes not fail whenUNCORRis nonzero or unknown, or whenSWEEPSis invalid. A failed or incomplete diagnostic can therefore exit successfully and be recorded as verified. Parse all 16 rows and return nonzero when the required invariants are not met.🤖 Prompt for 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. In `@docs/gps3-sessions/SESSION_LOG_20260729_storage.md` around lines 627 - 686, Update scripts/patrol_check.sh and the verification flow in verify_patrol_check.sh so all 16 drive rows are parsed and validation fails when any required health invariant is unmet: UNCORR must be known and zero, and SWEEPS must be present and valid. Propagate a nonzero status for missing, malformed, incomplete, or unhealthy rows instead of accepting patrol_check.sh’s output as verified; preserve the existing reporting and tee behavior.
🧹 Nitpick comments (2)
services/bernese-workflow/src/bernese_workflow/rinex_header_validator.py (1)
244-261: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBound the subprocess teardown.
proc.wait()has no timeout. Ifgzipdoes not exit afterSIGTERM, the scan blocks forever on a single file. Add a bounded wait with akill()fallback. This keeps the "exit status carries no information" discipline described in the comment.♻️ Proposed teardown
proc.stdout.close() proc.terminate() - proc.wait() + try: + proc.wait(timeout=5) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait()🤖 Prompt for 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. In `@services/bernese-workflow/src/bernese_workflow/rinex_header_validator.py` around lines 244 - 261, Update the subprocess teardown in the validator’s generator context around proc.stdout.close(), proc.terminate(), and proc.wait() to use a bounded wait; if the timeout expires, call proc.kill() and wait for termination to complete. Preserve the existing behavior of ignoring the subprocess exit status.services/bernese-workflow/tests/test_rinex_header_validator.py (1)
783-797: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe fixture is too small to exercise the SIGPIPE path.
The
.Zfixture holds a few hundred bytes.gzipwrites all output and exits before the context manager closes the pipe, so the early-exit teardown usually runs against an already-finished process. The test therefore does not reproduce the SIGPIPE/141 case it documents. Use a fixture larger than the pipe buffer (64 KiB on Linux) sogzipis still blocked inwrite()when the reader stops.Note that
_lzw_compress_smallrejects inputs that grow the dictionary past 512 entries, so a large fixture needs highly repetitive content or a different generator.🤖 Prompt for 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. In `@services/bernese-workflow/tests/test_rinex_header_validator.py` around lines 783 - 797, Update test_open_rinex_text_early_exit_on_dot_z_is_not_an_error so the compressed fixture expands beyond the pipe buffer while remaining compatible with _lzw_compress_small, using highly repetitive RINEX content or an appropriate generator. Preserve the existing first-line read and context-manager exit assertions, ensuring gzip is still writing when the reader abandons the stream and the SIGPIPE/141 path is exercised.
🤖 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 `@services/bernese-workflow/src/bernese_workflow/rinex_header_validator.py`:
- Around line 400-405: Update the _parse_rinex_headers docstring to describe the
current station-code precedence: prefer MARKER NUMBER, then use MARKER NAME only
when it is a bare four-character code, and finally fall back to the filename via
_resolve_station_code.
In `@services/bernese-workflow/tests/test_rinex_header_validator.py`:
- Around line 803-830: Add the existing pytest skip marker used by the other .Z
tests to test_parse_rinex_headers_reads_compressed_and_hatanaka, checking
shutil.which("gzip") and skipping when GNU gzip is unavailable. Keep the test
body and assertions unchanged.
---
Outside diff comments:
In `@docs/gps3-sessions/SESSION_LOG_20260729_storage.md`:
- Around line 725-756: Update census() in scripts/gps3_gpsdata_migrate.sh so
every find operation preserves and checks its exit status instead of redirecting
errors to /dev/null. Make any missing or unreadable path cause the census to
fail before emitting 0 counts or recording transfer state, while preserving
normal empty-directory results.
- Around line 489-538: Update scripts/gnss_mirror_update.sh to validate
core.logAllRefUpdates=true and gc.pruneExpire=never before running git remote
update. If either setting is missing or differs, fail closed with a clear error
and do not perform the mirror update; retain the existing mount and
noninteractive safeguards.
- Around line 627-686: Update scripts/patrol_check.sh and the verification flow
in verify_patrol_check.sh so all 16 drive rows are parsed and validation fails
when any required health invariant is unmet: UNCORR must be known and zero, and
SWEEPS must be present and valid. Propagate a nonzero status for missing,
malformed, incomplete, or unhealthy rows instead of accepting patrol_check.sh’s
output as verified; preserve the existing reporting and tee behavior.
---
Nitpick comments:
In `@services/bernese-workflow/src/bernese_workflow/rinex_header_validator.py`:
- Around line 244-261: Update the subprocess teardown in the validator’s
generator context around proc.stdout.close(), proc.terminate(), and proc.wait()
to use a bounded wait; if the timeout expires, call proc.kill() and wait for
termination to complete. Preserve the existing behavior of ignoring the
subprocess exit status.
In `@services/bernese-workflow/tests/test_rinex_header_validator.py`:
- Around line 783-797: Update
test_open_rinex_text_early_exit_on_dot_z_is_not_an_error so the compressed
fixture expands beyond the pipe buffer while remaining compatible with
_lzw_compress_small, using highly repetitive RINEX content or an appropriate
generator. Preserve the existing first-line read and context-manager exit
assertions, ensuring gzip is still writing when the reader abandons the stream
and the SIGPIPE/141 path is exercised.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1939299c-ae5e-4260-9f19-e2d62cf3e71c
📒 Files selected for processing (4)
docs/gps3-sessions/SESSION_LOG_20260729_storage.mddocs/project_documentation/bernese_orchestrator_r740_readiness.mdservices/bernese-workflow/src/bernese_workflow/rinex_header_validator.pyservices/bernese-workflow/tests/test_rinex_header_validator.py
Comment on lines
+400
to
+405
| station_code = _resolve_station_code( | ||
| filename_code=station_code, | ||
| marker_name=marker_name, | ||
| marker_number=marker_number, | ||
| source=p.name, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale _parse_rinex_headers docstring.
The docstring above (Lines 333-334) still states that the station code comes from MARKER NAME if present, else the filename. _resolve_station_code now prefers MARKER NUMBER, accepts MARKER NAME only when it is a bare 4-character code, and falls back to the filename. Align the docstring with the new precedence so readers do not rely on the old rule.
🤖 Prompt for 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.
In `@services/bernese-workflow/src/bernese_workflow/rinex_header_validator.py`
around lines 400 - 405, Update the _parse_rinex_headers docstring to describe
the current station-code precedence: prefer MARKER NUMBER, then use MARKER NAME
only when it is a bare four-character code, and finally fall back to the
filename via _resolve_station_code.
Comment on lines
+803
to
+830
| def test_parse_rinex_headers_reads_compressed_and_hatanaka(tmp_path): | ||
| """One session, four real-world encodings, four stations recovered.""" | ||
| src = tmp_path / "PGN" | ||
| src.mkdir() | ||
|
|
||
| (src / "paaa0010.24o").write_text(_rinex2_obs("PAAA"), encoding="ascii") | ||
|
|
||
| with _gzip.open(src / "pbbb0010.24d.gz", "wt", encoding="ascii") as fh: | ||
| fh.write(_crinex(_rinex2_obs("PBBB"))) | ||
|
|
||
| (src / "pccc0010.24o.Z").write_bytes( | ||
| _lzw_compress_small(_rinex2_obs("PCCC").encode("ascii")) | ||
| ) | ||
|
|
||
| with _gzip.open( | ||
| src / "PDDD00PHL_R_20240010000_01D_30S_MO.crx.gz", "wt", encoding="ascii" | ||
| ) as fh: | ||
| fh.write(_crinex(_rinex2_obs("PDDD"))) | ||
|
|
||
| # Must be ignored: navigation file for the same session. | ||
| with _gzip.open(src / "paaa0010.24n.gz", "wt", encoding="ascii") as fh: | ||
| fh.write(" 2.11 NAVIGATION DATA " | ||
| "RINEX VERSION / TYPE\n") | ||
|
|
||
| headers = _parse_rinex_headers(src, year=2024, session="0010") | ||
| assert set(headers) == {"PAAA", "PBBB", "PCCC", "PDDD"} | ||
| assert headers["PBBB"]["receiver"] == "LEICA GR50" # _rinex2_obs default | ||
| assert headers["PDDD"]["antenna"] == "LEIAR20 NONE" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the gzip skip guard to this test.
This test writes pccc0010.24o.Z and asserts that PCCC is recovered. The .Z path in _open_rinex_text runs GNU gzip. If the binary is absent, Popen raises FileNotFoundError, _parse_rinex_headers logs a warning and skips the file, and the assertion at Line 828 fails. The other .Z tests already carry @pytest.mark.skipif(shutil.which("gzip") is None, ...). Apply the same marker here for consistency.
🔧 Proposed guard
+@pytest.mark.skipif(shutil.which("gzip") is None, reason="GNU gzip required to read .Z")
def test_parse_rinex_headers_reads_compressed_and_hatanaka(tmp_path):
"""One session, four real-world encodings, four stations recovered."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_parse_rinex_headers_reads_compressed_and_hatanaka(tmp_path): | |
| """One session, four real-world encodings, four stations recovered.""" | |
| src = tmp_path / "PGN" | |
| src.mkdir() | |
| (src / "paaa0010.24o").write_text(_rinex2_obs("PAAA"), encoding="ascii") | |
| with _gzip.open(src / "pbbb0010.24d.gz", "wt", encoding="ascii") as fh: | |
| fh.write(_crinex(_rinex2_obs("PBBB"))) | |
| (src / "pccc0010.24o.Z").write_bytes( | |
| _lzw_compress_small(_rinex2_obs("PCCC").encode("ascii")) | |
| ) | |
| with _gzip.open( | |
| src / "PDDD00PHL_R_20240010000_01D_30S_MO.crx.gz", "wt", encoding="ascii" | |
| ) as fh: | |
| fh.write(_crinex(_rinex2_obs("PDDD"))) | |
| # Must be ignored: navigation file for the same session. | |
| with _gzip.open(src / "paaa0010.24n.gz", "wt", encoding="ascii") as fh: | |
| fh.write(" 2.11 NAVIGATION DATA " | |
| "RINEX VERSION / TYPE\n") | |
| headers = _parse_rinex_headers(src, year=2024, session="0010") | |
| assert set(headers) == {"PAAA", "PBBB", "PCCC", "PDDD"} | |
| assert headers["PBBB"]["receiver"] == "LEICA GR50" # _rinex2_obs default | |
| assert headers["PDDD"]["antenna"] == "LEIAR20 NONE" | |
| `@pytest.mark.skipif`(shutil.which("gzip") is None, reason="GNU gzip required to read .Z") | |
| def test_parse_rinex_headers_reads_compressed_and_hatanaka(tmp_path): | |
| """One session, four real-world encodings, four stations recovered.""" | |
| src = tmp_path / "PGN" | |
| src.mkdir() | |
| (src / "paaa0010.24o").write_text(_rinex2_obs("PAAA"), encoding="ascii") | |
| with _gzip.open(src / "pbbb0010.24d.gz", "wt", encoding="ascii") as fh: | |
| fh.write(_crinex(_rinex2_obs("PBBB"))) | |
| (src / "pccc0010.24o.Z").write_bytes( | |
| _lzw_compress_small(_rinex2_obs("PCCC").encode("ascii")) | |
| ) | |
| with _gzip.open( | |
| src / "PDDD00PHL_R_20240010000_01D_30S_MO.crx.gz", "wt", encoding="ascii" | |
| ) as fh: | |
| fh.write(_crinex(_rinex2_obs("PDDD"))) | |
| # Must be ignored: navigation file for the same session. | |
| with _gzip.open(src / "paaa0010.24n.gz", "wt", encoding="ascii") as fh: | |
| fh.write(" 2.11 NAVIGATION DATA " | |
| "RINEX VERSION / TYPE\n") | |
| headers = _parse_rinex_headers(src, year=2024, session="0010") | |
| assert set(headers) == {"PAAA", "PBBB", "PCCC", "PDDD"} | |
| assert headers["PBBB"]["receiver"] == "LEICA GR50" # _rinex2_obs default | |
| assert headers["PDDD"]["antenna"] == "LEIAR20 NONE" |
🤖 Prompt for 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.
In `@services/bernese-workflow/tests/test_rinex_header_validator.py` around lines
803 - 830, Add the existing pytest skip marker used by the other .Z tests to
test_parse_rinex_headers_reads_compressed_and_hatanaka, checking
shutil.which("gzip") and skipping when GNU gzip is unavailable. Keep the test
body and assertions unchanged.
Readiness §5 step 2 says "provision $U from repo gold-standard
PCFs/panels/scripts". The gold standard did not exist. Checking first
was worthwhile: gps3's $U/OPT, PCF, SCRIPT and PAN are byte-identical to
the $C/USER template shipped with Bernese 5.4 — zero files differing, so
nothing PHIVOLCS-specific had ever been deployed there — and the repo
held only pagenet_pcs.pl plus one Jinja template.
Adds config/bernese/gpsuser/ as the source of truth and
scripts/provision_gpsuser.py to apply it. This is what readiness §4
actually requires: after the MIS team reconfigures the box, a working
environment must be recoverable by re-running provisioning rather than
by someone re-debugging panels from memory.
Three file classes, handled deliberately differently:
OPT/**/*.INP separator-sanitized, ADDNEQ2 MAXPAR sized from the
station count. Windows backslashes are literal
characters on Linux.
SCRIPT/* copied verbatim. A backslash in Perl is an escape, not
a path separator; converting it corrupts the driver.
PCF/*.PCF refused if they carry a dangling WAIT. A WAIT on an
undefined PID does not fail loudly — the BPE waits for
a process that will never run, forever.
PAN/USER.CPU is generated from detected hardware and deliberately NOT
versioned. maxjobs must track the host's physical cores, so a committed
copy would carry one machine's count onto another — which is precisely
how gps3 came to be running the T420's maxjobs 2 on a 12-core server.
The provisioner detected the hardware independently and arrived at 11,
matching the value set by hand earlier.
Dry run by default. Strict: a panel carrying an unresolvable hazard
aborts the whole run before anything is written, so $U is never left
half-updated. Applied on gps3 — pagenet_pcs.pl is deployed
byte-identical and a second run reports no changes.
Still blocked: PAGENET_DLY.PCF exists only on the T420 and must be
captured, not re-derived. It is RNX2SNX modules 1-14 (PID 001->514), but
that is not a truncation anyone can safely perform by eye — 599 DUMMY
waits on 512 514 522, so dropping the R2S_RED branch leaves 599 waiting
on a PID that never runs, the exact dangling WAIT this provisioner now
refuses. A re-derived PCF would not be the one validated during the
training week, and the acceptance test would be exercising something
nobody has ever run.
Adds §15 (end-of-session state), fixes the section numbering, and gives the document a reading guide. §15 records what actually changed on the machine as distinct from what was merely written down — maxjobs 2 -> 11, pagenet_pcs.pl deployed, patrol_check.sh corrected and moved into the repo, the scripts/sudo/ convention — and notes explicitly that nothing on the array, the mirror or the archive was touched. It lists the seven questions that were open in the morning and are now closed, and isolates the single remaining blocker: PAGENET_DLY.PCF exists only on the T420. §15.5 names the shape this project keeps hitting, because it will recur: a check that reports success without having inspected anything. Five instances now — smartd's DEVICESCAN monitoring zero drives, SIGPIPE inverting a pipeline's status, patrol_check.sh reading transposed columns, then reading the right column in the wrong units, and validate_rinex_headers() returning a passing report having read no files. In every case the output was individually plausible, and in four of the five a green test suite or a zero exit status concealed it. What broke each one was the same move: go back to the raw output of the underlying tool rather than iterating on the summary. Housekeeping: 14.5/14.6/14.7 renumbered to 14.4/14.5/14.6, closing a gap left by an earlier edit, with cross-references updated. §10 dated, since §15 is also an end-of-session section from a different day. The title now spans 07-29 to 08-03 rather than claiming to be about storage provisioning, and a short table up front points a newcomer at the four sections worth reading first — the log is past 1,100 lines and is the succession record, so navigability is part of its job.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/gps3-sessions/SESSION_LOG_20260729_storage.md`:
- Around line 916-919: Disambiguate the test totals in
docs/gps3-sessions/SESSION_LOG_20260729_storage.md:916-919 by labeling 179 as
the validator-suite count, and in
docs/gps3-sessions/SESSION_LOG_20260729_storage.md:1097-1108 by labeling 189 as
the repository-wide total; correct either value if necessary while preserving
the reported validation results.
- Around line 1082-1084: Revise the `$U` gold-standard statement in the session
summary to avoid claiming completeness. Mention that provisioning is available
and repository assets are partial, while `PAGENET_DLY.PCF` remains T420-only and
prevents acceptance testing on gps3.
- Around line 1066-1068: Clarify the scope of the read-only statement near the
session summary by limiting it to interactive work during the final session,
unless mirror cron activity is explicitly included. Keep it consistent with the
mirror updates documented in §13.1 and §13.7.
In `@scripts/provision_gpsuser.py`:
- Around line 292-308: Update the final return logic in the PAGENET readiness
reporting flow to return failure when the missing list from
check_pagenet_readiness indicates incomplete readiness, including a missing
PAGENET_DLY.PCF. Preserve the existing all_errors failure behavior and success
result when both checks pass.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3d94e18-8efb-415e-82b9-62b61e0ceb28
📒 Files selected for processing (6)
config/bernese/gpsuser/README.mdconfig/bernese/gpsuser/SCRIPT/pagenet_pcs.pldocs/gps3-sessions/SESSION_LOG_20260729_storage.mddocs/project_documentation/bernese_orchestrator_r740_readiness.mdscripts/provision_gpsuser.pyservices/bernese-workflow/tests/test_provision_gpsuser.py
Comment on lines
+916
to
+919
| ### 14.4 C2 fixed — the validator now sees the real DATAPOOL | ||
|
|
||
| **Result: all seven PAGENET sessions (DOY 084–090) validate clean**, from a | ||
| starting point of zero files visible. 179 tests pass, up from 128. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Disambiguate the test totals.
The log reports 179 tests in one section and 189 tests in another. State that 179 is the validator-suite count and 189 is the repository-wide count, or correct one of the values.
docs/gps3-sessions/SESSION_LOG_20260729_storage.md#L916-L919: Label the 179-test result with its scope.docs/gps3-sessions/SESSION_LOG_20260729_storage.md#L1097-L1108: Label the 189-test result as the repository-wide total.
📍 Affects 1 file
docs/gps3-sessions/SESSION_LOG_20260729_storage.md#L916-L919(this comment)docs/gps3-sessions/SESSION_LOG_20260729_storage.md#L1097-L1108
🤖 Prompt for 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.
In `@docs/gps3-sessions/SESSION_LOG_20260729_storage.md` around lines 916 - 919,
Disambiguate the test totals in
docs/gps3-sessions/SESSION_LOG_20260729_storage.md:916-919 by labeling 179 as
the validator-suite count, and in
docs/gps3-sessions/SESSION_LOG_20260729_storage.md:1097-1108 by labeling 189 as
the repository-wide total; correct either value if necessary while preserving
the reported validation results.
Comment on lines
+1066
to
+1068
| **Nothing was changed on the array, the mirror, or the archive.** The storage | ||
| side was read-only this session. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '500,570p' docs/gps3-sessions/SESSION_LOG_20260729_storage.md
sed -n '1052,1068p' docs/gps3-sessions/SESSION_LOG_20260729_storage.mdRepository: alfieprojectsdev/movefaults
Length of output: 4576
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '700,800p' docs/gps3-sessions/SESSION_LOG_20260729_storage.md
sed -n '850,940p' docs/gps3-sessions/SESSION_LOG_20260729_storage.md
sed -n '1000,1100p' docs/gps3-sessions/SESSION_LOG_20260729_storage.mdRepository: alfieprojectsdev/movefaults
Length of output: 15244
Clarify whether the read-only statement excludes mirror cron activity.
§13.1 and §13.7 record mirror cron updates during the unattended period before the 2026-08-03 resume. If “nothing was changed on the array, the mirror, or the archive” only applies to interactive work on 2026-08-03, restrict it to the final session. Otherwise it conflicts with the recorded mirror updates.
🤖 Prompt for 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.
In `@docs/gps3-sessions/SESSION_LOG_20260729_storage.md` around lines 1066 - 1068,
Clarify the scope of the read-only statement near the session summary by
limiting it to interactive work during the final session, unless mirror cron
activity is explicitly included. Keep it consistent with the mirror updates
documented in §13.1 and §13.7.
Comment on lines
+292
to
+308
| missing = check_pagenet_readiness(args.user) | ||
| print("\nPAGENET acceptance-test readiness:") | ||
| if missing: | ||
| for m in missing: | ||
| print(f" {Colors.ERR}MISSING {m}{Colors.OFF}") | ||
| print( | ||
| f"\n {Colors.DIM}See config/bernese/gpsuser/README.md — " | ||
| f"PAGENET_DLY.PCF must be captured from the T420, not re-derived." | ||
| f"{Colors.OFF}" | ||
| ) | ||
| else: | ||
| print(f" {Colors.OK}all required assets present{Colors.OFF}") | ||
|
|
||
| if not args.apply and all_actions: | ||
| print(f"\n{Colors.DIM}Re-run with --apply to write.{Colors.OFF}") | ||
|
|
||
| return 1 if all_errors else 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return failure when PAGENET readiness is incomplete.
When PAGENET_DLY.PCF is missing, this code prints MISSING but returns 0. An automated acceptance gate can then continue although the documented required asset is absent.
Proposed fix
- return 1 if all_errors else 0
+ return 1 if all_errors or missing else 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| missing = check_pagenet_readiness(args.user) | |
| print("\nPAGENET acceptance-test readiness:") | |
| if missing: | |
| for m in missing: | |
| print(f" {Colors.ERR}MISSING {m}{Colors.OFF}") | |
| print( | |
| f"\n {Colors.DIM}See config/bernese/gpsuser/README.md — " | |
| f"PAGENET_DLY.PCF must be captured from the T420, not re-derived." | |
| f"{Colors.OFF}" | |
| ) | |
| else: | |
| print(f" {Colors.OK}all required assets present{Colors.OFF}") | |
| if not args.apply and all_actions: | |
| print(f"\n{Colors.DIM}Re-run with --apply to write.{Colors.OFF}") | |
| return 1 if all_errors else 0 | |
| missing = check_pagenet_readiness(args.user) | |
| print("\nPAGENET acceptance-test readiness:") | |
| if missing: | |
| for m in missing: | |
| print(f" {Colors.ERR}MISSING {m}{Colors.OFF}") | |
| print( | |
| f"\n {Colors.DIM}See config/bernese/gpsuser/README.md — " | |
| f"PAGENET_DLY.PCF must be captured from the T420, not re-derived." | |
| f"{Colors.OFF}" | |
| ) | |
| else: | |
| print(f" {Colors.OK}all required assets present{Colors.OFF}") | |
| if not args.apply and all_actions: | |
| print(f"\n{Colors.DIM}Re-run with --apply to write.{Colors.OFF}") | |
| return 1 if all_errors or missing else 0 |
🤖 Prompt for 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.
In `@scripts/provision_gpsuser.py` around lines 292 - 308, Update the final return
logic in the PAGENET readiness reporting flow to return failure when the missing
list from check_pagenet_readiness indicates incomplete readiness, including a
missing PAGENET_DLY.PCF. Preserve the existing all_errors failure behavior and
success result when both checks pass.
The survival table previously covered tmux and the conversation only, and said nothing about whether a remote-controlled session survives losing its terminal — which is the property that matters for the archive transfer. Measured 2026-08-03 rather than inferred. With the SSH window closed and `tmux list-clients` reporting no attached clients, the session still executed commands and responded. Confirmed as continuation rather than restart by PID: the process kept its original number with an uptime matching the tmux session's creation time. Practical consequence: a remote-controlled session needs no terminal watching it, so closing the laptop does not interrupt work in progress. The reboot row is unchanged and still the one that bites — tmux keeps nothing on disk. Caveat recorded with the result: a second SSH session from another host was open during the test. The tmux session had zero attached clients regardless, so the finding holds, but the test was not clean-room.
A station present in the RINEX but absent from the campaign .STA makes RXOBV3 hard-abort the whole cluster — the failure that killed DOY 086 during the training week. The workaround applied then was to move the files into a hidden quarantine directory, which discards the station's data. This adds the station properly instead. Derives everything obtainable from a RINEX header — marker, DOMES, receiver and antenna types and serials, approximate XYZ — and emits rows for STA (TYPE 001 and 002), CRD, VEL, ABB and CLU. It clones an existing row and overwrites known field ranges rather than formatting rows from scratch. These files are fixed-column and a STA TYPE 002 row is 272 characters across fourteen fields; one column of drift silently moves a receiver serial into the antenna field and Bernese reads the result without complaint. Field offsets come from each file's own **** ruler line. That was the stated intent from the start but not the first implementation, which hardcoded them and got the ABB 2-ID at column 33 where the ruler says 34. The uniqueness check then compared ' C' against real 2-IDs, matched nothing, and emitted a three-character ID into a two-character field. Parsing the ruler removes the class of error rather than the instance. Velocity is not in a RINEX header, so it is seeded from the nearest existing station and labelled an estimate needing confirmation. For PLG2 that is PSRG at 37.4 km, both on the Eurasian plate. Dry run by default; --apply backs up every file it touches first. Not yet applied to PLG2 — seeding an a priori velocity is a call for a geodesist, not for tooling.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
scripts/add_station_to_campaign.py (1)
1-370: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd test coverage for the fixed-width row generation.
This tool performs non-trivial fixed-width parsing and column-offset arithmetic against production Bernese reference files, with no accompanying test file (the sibling GPSUSER-provisioning layer in this PR has
test_provision_gpsuser.py; this tool has none). Given the correctness ofruler_fields,_overwrite, and the per-filebuild_*_rowfunctions is exactly what determines whether--applycorrupts or correctly extendsSTA/CRD/VEL/ABB/CLU, unit tests against representative fixture files (including aPGN.STA-shaped block with FLG/FROM/TO columns) would catch the kind of column-index bug flagged above before it reaches--apply.🤖 Prompt for 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. In `@scripts/add_station_to_campaign.py` around lines 1 - 370, Add unit tests for ruler_fields, _overwrite, and each build_*_row function using representative Bernese fixtures, including a PGN.STA-shaped block with FLG/FROM/TO columns. Verify generated rows preserve fixed widths and place station metadata in the correct ruler-derived fields across STA, CRD, VEL, ABB, and CLU, including ABB 2-ID uniqueness behavior.
🤖 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/add_station_to_campaign.py`:
- Around line 323-328: Update the membership check used to build present in the
station-registration flow so meta.name is matched as a standalone, word-bounded
marker rather than a plain substring. Apply the same bounded matching
consistently across each file’s decoded text, preserving the existing present
reporting and early return behavior.
- Around line 355-363: Update the apply loop to reuse the in-memory file
contents created during planning (including sta_text, crd_text, and the
per-extension lines from the planning phase) instead of rereading files. Before
modifying any file, validate that every existing content and planned row is
ASCII-encodable, then perform writes only after all validation succeeds so
encoding failures cannot leave the multi-file update partially applied.
- Around line 183-208: Update build_crd_row and build_vel_row to obtain field
offsets from the file’s **** ruler line via ruler_fields(), using those ranges
for station number, name, and CRD coordinate fields instead of hardcoded
positions. Preserve donor velocity values and existing row-number/row-insertion
behavior while applying the derived ranges to both CRD and VEL rows.
- Around line 67-83: The .Z handling in _open_text must preserve the subprocess
result so read_station_meta can detect gzip failures after consuming the header.
Return or otherwise retain the Popen object for the gzip path, then check its
completion status after header parsing and raise or propagate an error when the
decompressor exits non-zero instead of accepting partial metadata.
- Around line 156-159: Update _STA_ROW and the template-selection logic in
build_sta_rows to avoid identifying STA rows by the literal 001 value. Use the
ruler-derived text position to select the last TYPE 001/002 data row, then
rebuild its marker, name, and header fields for the generated row. Preserve
support for rows with FLG values such as 002 or 003 and avoid raising IndexError
when those are the only matching templates.
---
Nitpick comments:
In `@scripts/add_station_to_campaign.py`:
- Around line 1-370: Add unit tests for ruler_fields, _overwrite, and each
build_*_row function using representative Bernese fixtures, including a
PGN.STA-shaped block with FLG/FROM/TO columns. Verify generated rows preserve
fixed widths and place station metadata in the correct ruler-derived fields
across STA, CRD, VEL, ABB, and CLU, including ABB 2-ID uniqueness behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bd88616-9bb7-454b-8875-828047d234ef
📒 Files selected for processing (2)
docs/gps3_tmux_claude_runbook.mdscripts/add_station_to_campaign.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/gps3_tmux_claude_runbook.md
Comment on lines
+67
to
+83
| def _open_text(path: Path): | ||
| """Read a RINEX file, plain or compressed. Mirrors the validator's handling. | ||
|
|
||
| A CRINEX (Hatanaka) file stores the original header verbatim after two | ||
| CRINEX lines, so no crx2rnx is needed to read what we want here. | ||
| """ | ||
| s = path.suffix.lower() | ||
| if s == ".gz": | ||
| return gzip.open(path, "rt", encoding="ascii", errors="replace") | ||
| if s == ".z": | ||
| # Python has no LZW decoder; GNU gzip reads both formats. | ||
| return subprocess.Popen( | ||
| ["gzip", "-dc", str(path)], | ||
| stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, | ||
| text=True, encoding="ascii", errors="replace", | ||
| ).stdout | ||
| return path.open(encoding="ascii", errors="replace") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)add_station_to_campaign\.py$|scripts/' || true
echo "== target file outline/sections =="
if [ -f scripts/add_station_to_campaign.py ]; then
wc -l scripts/add_station_to_campaign.py
sed -n '1,140p' scripts/add_station_to_campaign.py
fi
echo "== usages of _open_text and read_station_meta =="
rg -n "_open_text|read_station_meta|gzip -dc|Popen" .Repository: alfieprojectsdev/movefaults
Length of output: 9611
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== validator subprocess handling =="
if [ -f services/bernese-workflow/src/bernese_workflow/rinex_header_validator.py ]; then
wc -l services/bernese-workflow/src/bernese_workflow/rinex_header_validator.py
sed -n '200,275p' services/bernese-workflow/src/bernese_workflow/rinex_header_validator.py
fi
echo "== rest of add_station_to_campaign usage =="
sed -n '250,340p' scripts/add_station_to_campaign.py
echo "== python subprocess behavior with short gzip/decompressing stream =="
python3 - <<'PY'
import subprocess
try:
proc = subprocess.Popen(["sh", "-c", "printf 'abc'", stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True])
stdout = proc.stdout.read(3)
print("bytes", stdout, proc.returncode)
print("wait", proc.wait(), proc.returncode)
except Exception as e:
print("exception", repr(e))
try:
proc = subprocess.Popen(["sh", "-c", "printf 'abc'>&1; exit 1", stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True] )
stdout = proc.stdout.read(3)
print("exit1 bytes", stdout, proc.returncode)
print("wait", proc.wait(), proc.returncode)
except Exception as e:
print("exception", repr(e))
try:
proc = subprocess.Popen(["sh", "-c", "printf 'abc'; exit 2", stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, text=True])
stdout = proc.stdout.read(3)
print("exit2 bytes", stdout, proc.returncode)
print("wait", proc.wait(), proc.returncode)
except Exception as e:
print("exception", repr(e))
PYRepository: alfieprojectsdev/movefaults
Length of output: 7741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== validator subprocess handling (continued) =="
sed -n '228,258p' services/bernese-workflow/src/bernese_workflow/rinex_header_validator.py
echo "== remainder of read_station_meta =="
sed -n '86,120p' scripts/add_station_to_campaign.py
echo "== python subprocess behavior with short gzip/decompressing stream =="
python3 - <<'PY'
import subprocess
import os
import signal
def open_proc(shellspec, stderr_devnull):
proc = subprocess.Popen(
["sh", "-c", shellspec],
stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL if stderr_devnull else subprocess.PIPE,
text=True,
)
return proc
for label, shellspec in (
("partial", "printf 'abc'"),
("exit1", "printf 'abc'>&1; exit 1"),
("exit2", "printf 'abc'; exit 2"),
("SIGPIPE", "sleep 0.2"),
):
proc = open_proc(shellspec, stderr_devnull=False)
stdout = proc.stdout.read(3)
print(f"{label}: stdout={stdout!r}; returncode={proc.returncode}; wait={proc.wait()}; returncode={proc.returncode}")
proc = open_proc("printf 'abc'; sleep 0.2", stderr_devnull=False)
stdout = proc.stdout.read(3)
print("SIGPIPE_readonly:", stdout, proc.returncode)
proc.pid and os.kill(proc.pid, signal.SIGPIPE)
os.waitpid(proc.pid, 0)
print("after SIGPIPE:", proc.returncode)
PYRepository: alfieprojectsdev/movefaults
Length of output: 3334
Detect .Z decompression failures before parsing the header.
read_station_meta() can return metadata from an incomplete .Z stream when gzip -dc writes partial output and exits non-zero. _open_text() discards the Popen object, so iteration cannot call wait() or read stderr; return the Popen and check/delegate returncode after reading the header, or surface a failed decompression as an error.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 77-81: Command coming from incoming request
Context: subprocess.Popen(
["gzip", "-dc", str(path)],
stdout=subprocess.PIPE, stderr=subprocess.DEVNULL,
text=True, encoding="ascii", errors="replace",
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.0)
[error] 78-78: subprocess call: check for execution of untrusted input
(S603)
[error] 79-79: Starting a process with a partial executable path
(S607)
🤖 Prompt for 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.
In `@scripts/add_station_to_campaign.py` around lines 67 - 83, The .Z handling in
_open_text must preserve the subprocess result so read_station_meta can detect
gzip failures after consuming the header. Return or otherwise retain the Popen
object for the gzip path, then check its completion status after header parsing
and raise or propagate an error when the decompressor exits non-zero instead of
accepting partial metadata.
Source: Linters/SAST tools
Comment on lines
+156
to
+159
| _CRD_ROW = re.compile(r"^\s*\d+\s+[A-Z0-9]{4}\s") | ||
| _ABB_ROW = re.compile(r"^[A-Z0-9]{4}\s") | ||
| _CLU_ROW = re.compile(r"^[A-Z0-9]{4}\s") | ||
| _STA_ROW = re.compile(r"^[A-Z0-9]{4}\s.*\s001\s") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline scripts/add_station_to_campaign.py || true
echo "== relevant sections =="
sed -n '1,240p' scripts/add_station_to_campaign.py | cat -n
echo "== repository references to add_station_to_campaign and STA handling =="
rg -n "add_station_to_campaign|build_sta_rows|_STA_ROW|TYPE 001|TYPE 002|PGN\.STA|001|STA_ROW|FLG|ruler_fields" .Repository: alfieprojectsdev/movefaults
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target script size and relevant source =="
wc -l scripts/add_station_to_campaign.py
sed -n '1,230p' scripts/add_station_to_campaign.py | cat -n
echo "== references in tracked source/config docs only (exclude likely mass data paths) =="
python3 - <<'PY'
import subprocess, pathlib
cmd = subprocess.run(['git','ls-files'], text=True, capture_output=True)
files = cmd.stdout.splitlines()
refs = subprocess.run(['rg','-n','add_station_to_campaign|build_sta_rows|_STA_ROW|TYPE 001|TYPE 002|PGN\.STA|001|STA_ROW|FLG|ruler_fields'],
text=True,capture_output=True,input='\n'.join(files), env={'LC_ALL':'C'})
for line in refs.stdout.splitlines()[:200]:
print(line)
print('total lines', refs.stdout.count('\n'))
PY
echo "== focused regex behavior probe =="
python3 - <<'PY'
import re
pattern = re.compile(r"^[A-Z0-9]{4}\s.*\s001\s")
samples = [
"ABCD xxxxx xxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx 001 xxxxxxxxxxx",
"ABCD xxxxx xxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx 002 xxxxxxxxxxx",
"ABCD xxxxx xxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx 001xxx",
"ABCD xxxxxxxxx 001 xxxxxxxxxxx",
"1234 ABCD 001 ",
]
for s in samples:
print(repr(s), bool(pattern.match(s)))
PYRepository: alfieprojectsdev/movefaults
Length of output: 11541
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== remaining script sections =="
sed -n '231,370p' scripts/add_station_to_campaign.py | cat -n
echo "== sample PGN.STA/STA references in repository data/docs =="
python3 - <<'PY'
import subprocess, pathlib, glob
cmd = subprocess.run(['git','ls-files'], text=True, capture_output=True)
files = cmd.stdout.splitlines()
pat = ['*STA*','*PGN*','stations.dat','station']
matches = []
for pattern in pat:
matches += glob.glob(pattern, recursive=True)
for path in matches[:40]:
if pathlib.Path(path).is_file():
print("PATH", path, "lines", sum(1 for _ in open(path, encoding='ascii', errors='replace')), "has 001", " 001 " in open(path, encoding='ascii', errors='replace').read()[:100000])
PY
echo "== deterministic search for all 001 token occurrences in source/scripts/config =="
rg -n --smart-case --glob '!services/vadase-rt-monitor/data/**' --glob '!**/NMEA_*' --glob '!services/**/data/**' '\b001\b' scripts services migrations README.md docs .github 2>/dev/null | head -n 200 || trueRepository: alfieprojectsdev/movefaults
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== remaining script sections =="
sed -n '231,370p' scripts/add_station_to_campaign.py | cat -n
echo "== sample PGN.STA/STA references in repository data/docs =="
python3 - <<'PY'
import subprocess, pathlib, glob
cmd = subprocess.run(['git','ls-files'], text=True, capture_output=True)
files = cmd.stdout.splitlines()
pat = ['*STA*','*PGN*','stations.dat','station']
matches = []
for candidate in pat:
matches += glob.glob(candidate, recursive=True)
seen = set()
for path in matches[:80]:
if path in seen:
continue
seen.add(path)
p = pathlib.Path(path)
if p.is_file():
try:
text = open(p, encoding='ascii', errors='replace').read(100000)
print(f"PATH {path} lines {sum(1 for _ in open(p, encoding='ascii', errors='replace'))} has 001 {' 001 ' in text}")
except Exception as exc:
print(f"PATH {path} ERROR {exc}")
PY
echo "== deterministic search for all 001 token occurrences in source/scripts/config/docs =="
rg -n --smart-case --glob '!services/vadase-rt-monitor/data/**' --glob '!**/NMEA_*' --glob '!services/**/data/**' '\b001\b' scripts services migrations README.md docs .github 2>/dev/null | head -n 200 || trueRepository: alfieprojectsdev/movefaults
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== read target script safely == "
python3 - <<'PY'
from pathlib import Path
p=Path('scripts/add_station_to_campaign.py')
text=p.read_text(encoding='ascii',errors='replace').splitlines()
for i,line in enumerate(text[152:162], start=157):
print(f'{i}: {line}')
for i,line in enumerate(text[243:268], start=244):
print(f'{i}: {line}')
PY
echo "== repository file list candidates =="
git ls-files | rg '(^|/)(README|docs|scripts|pgn|STA|stations|config).*|\.md$|\.rst$' | head -n 300
echo "== targeted source-only references for PGN.STA generation =="
rg -n --glob '!docs/*.html' --glob '!docs/**/*.html' --glob '!services/vadase-rt-monitor/data/**' --glob '!**/NMEA_*' 'TYPE 00[0-9]|TYPE 002|TYPE 003|STATION NAME|RECEIVER TYPE|FLAG|FLG|001\s+\S{19}' scripts services . 2>/dev/null | head -n 200Repository: alfieprojectsdev/movefaults
Length of output: 23835
Do not select the STA template by the literal 001.
_STA_ROW requires a whitespace-bounded 001, but PGN.STA can have additional TYPE 001/002 rows with other FLG values such as 002 or 003. When those rows are later in the block, build_sta_rows clones the wrong template; if no matching 001 row exists, it raises IndexError. Select the template row via its ruler-derived text position, for example taking the last TYPE 001/002 data row and rebuilding the marker/name/header fields instead of matching 001.
🤖 Prompt for 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.
In `@scripts/add_station_to_campaign.py` around lines 156 - 159, Update _STA_ROW
and the template-selection logic in build_sta_rows to avoid identifying STA rows
by the literal 001 value. Use the ruler-derived text position to select the last
TYPE 001/002 data row, then rebuild its marker, name, and header fields for the
generated row. Preserve support for rows with FLG values such as 002 or 003 and
avoid raising IndexError when those are the only matching templates.
Comment on lines
+183
to
+208
| def build_crd_row(lines: list[str], meta: StationMeta) -> tuple[int, str]: | ||
| rows = _data_rows(lines, _CRD_ROW) | ||
| idx, template = rows[-1] | ||
| num = max(int(re.match(r"\s*(\d+)", ln).group(1)) for _, ln in rows) + 1 | ||
| row = template | ||
| row = _overwrite(row, 0, 3, f"{num:>3}") | ||
| row = _overwrite(row, 5, 16, meta.full_name) | ||
| row = _overwrite(row, 21, 15, f"{meta.x:15.5f}") | ||
| row = _overwrite(row, 36, 15, f"{meta.y:15.5f}") | ||
| row = _overwrite(row, 51, 15, f"{meta.z:15.5f}") | ||
| return idx + 1, row.rstrip() | ||
|
|
||
|
|
||
| def build_vel_row( | ||
| lines: list[str], meta: StationMeta, donor: str | ||
| ) -> tuple[int, str]: | ||
| rows = _data_rows(lines, _CRD_ROW) | ||
| donor_rows = [ln for _, ln in rows if f" {donor} " in ln] | ||
| if not donor_rows: | ||
| raise ValueError(f"donor station {donor} not present in VEL") | ||
| idx = rows[-1][0] | ||
| num = max(int(re.match(r"\s*(\d+)", ln).group(1)) for _, ln in rows) + 1 | ||
| row = donor_rows[0] # keep the donor's velocity values | ||
| row = _overwrite(row, 0, 3, f"{num:>3}") | ||
| row = _overwrite(row, 5, 16, meta.full_name) | ||
| return idx + 1, row.rstrip() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching add_station/campaign =="
fd -i 'add_station|campaign' . || true
echo "== outline =="
ast-grep outline scripts/add_station_to_campaign.py --view compact || true
echo "== relevant source sections =="
sed -n '1,240p' scripts/add_station_to_campaign.py | cat -n
echo "== rulers/readers references =="
rg -n "ruler_fields|CRD_ROW|_data_rows|def build_(abb|clu|sta|crd|vel)_row|def .*CRD|def .*VEL" scripts/add_station_to_campaign.pyRepository: alfieprojectsdev/movefaults
Length of output: 13192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src and writer references =="
sed -n '240,420p' scripts/add_station_to_campaign.py | cat -n
echo "== ruler/field lookup tests or fixture references =="
rg -n "ruler_fields|max_len|expected ruler|CRD|VEL|****|type_001|type_002" scripts tests .github --glob '!**/results/**' --glob '!**/data/**' || true
echo "== Python parse/data-shape probe =="
python3 - <<'PY'
import ast, pathlib
p = pathlib.Path('scripts/add_station_to_campaign.py')
src = p.read_text()
tree = ast.parse(src)
def call_names(func):
calls = []
for node in ast.walk(func):
if isinstance(node, ast.Call):
if isinstance(node.func, ast.Name):
calls.append(node.func.id)
return calls
for node in tree.body:
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name.startswith('build_'):
print(f"{node.name}: ruler_fields calls={('ruler_fields',) in call_names(node)}")
calls = call_names(node)
if 'ruler_fields' not in calls:
print(" missing ruler_fields, call names:", calls)
PY
echo "== find existing CRD/VEL sample files if any =="
git ls-files | rg -i '(\.crd|\.vel|campaign|ref|station)' || trueRepository: alfieprojectsdev/movefaults
Length of output: 8707
Use ruler_fields() for CRD and VEL field offsets.
build_crd_row() and build_vel_row() hardcode (0,3), (5,16), (21,15), (36,15), and (51,15), while the module document says fixed-column files should derive ranges from the file’s own **** ruler line. If the actual CRD/VEL ruler differs, these rows will drift into the wrong columns for two of the five reference files.
🤖 Prompt for 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.
In `@scripts/add_station_to_campaign.py` around lines 183 - 208, Update
build_crd_row and build_vel_row to obtain field offsets from the file’s ****
ruler line via ruler_fields(), using those ranges for station number, name, and
CRD coordinate fields instead of hardcoded positions. Preserve donor velocity
values and existing row-number/row-insertion behavior while applying the derived
ranges to both CRD and VEL rows.
Comment on lines
+355
to
+363
| stamp = datetime.now().strftime("%Y%m%d-%H%M%S") | ||
| for ext in REF_FILES: | ||
| path = files[ext] | ||
| shutil.copy2(path, path.with_suffix(f".{ext}.bak-{stamp}")) | ||
| lines = path.read_text(encoding="ascii", errors="replace").splitlines() | ||
| for offset, (idx, row) in enumerate(sorted(planned[ext])): | ||
| lines.insert(idx + offset, row) | ||
| path.write_text("\n".join(lines) + "\n", encoding="ascii") | ||
| print(f" wrote {path.name} (backup .bak-{stamp})") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reuse the already-read content and guard against partial multi-file writes.
Two related gaps in the apply loop:
lines = path.read_text(...)at line 359 re-reads each file from disk, discarding the content already read during planning (sta_textat 335,crd_textat 323, and the per-extlinesbuilt at 343). Between the two reads there is a time-of-check/time-of-use gap: if anything else touches these files in between (another operator, a concurrent BPE run), the insertion indices computed during planning no longer match the freshly-read content, andlines.insert(idx + offset, row)can insert at the wrong position.- All files are read with
errors="replace"but written withencoding="ascii"(strict). If any existing file content is not pure ASCII, the read silently substitutes U+FFFD, and the write at line 362 raisesUnicodeEncodeError. Since the loop writes STA, CRD, VEL, ABB, CLU in sequence and a failure partway through leaves earlier files already modified (with backups) but later files untouched, an encoding failure mid-loop leaves the reference file set in an inconsistent, partially-registered state — a state that could confuse Bernese in a different way than the original missing-station problem.
Reusing the in-memory content computed during planning removes the TOCTOU gap and the redundant I/O. Validating that all planned row content is ASCII-clean before starting any write (or writing to temp files and renaming only after all writes succeed) would remove the partial-update risk.
🤖 Prompt for 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.
In `@scripts/add_station_to_campaign.py` around lines 355 - 363, Update the apply
loop to reuse the in-memory file contents created during planning (including
sta_text, crd_text, and the per-extension lines from the planning phase) instead
of rereading files. Before modifying any file, validate that every existing
content and planned row is ASCII-encodable, then perform writes only after all
validation succeeds so encoding failures cannot leave the multi-file update
partially applied.
An audit of docs/ found three different problems wearing the same
"out of date" label. Each needed a different fix.
deliverables_tracker.md — the live status document, five references,
last updated 2026-07-01 and therefore predating everything done on the
R740. BRN-001 was still listed as open and "gated on MIS access"; it
completed on 2026-07-29 with the EXAMPLE campaign verified at 0.0000 mm.
P0 items 1-4 are done, P1 item 7 is half done (USER.CPU sized, V_CLUFIN
still untuned).
Two corrections rather than additions. The note claiming the station
validator "prevents the PLG2 hard-abort" was wrong in a way worth
recording: it detects the condition, it does not fix it, and PLG2 is
still missing from all five PGN.* reference files with its data
hand-quarantined. And P0 item 1 is marked complete with the caveat that
"complete" concealed a validator returning passing reports having read
nothing — the 128 tests passed throughout because their fixtures used a
filename space that does not occur in production. Also dropped the
"47-step BPE" figure, which matches no count in 5.4's RNX2SNX.PCF
(64 PIDs, 51 unique scripts, 50 PIDs <=514 excluding DUMMY); it may be
correct for the PHIVOLCS 5.2 PCF, so it is removed rather than replaced
with a guess.
bernese_orchestration_explainer.md — staff-facing, and its errors were
the kind colleagues would act on. "What We Need From You" asked for
USER.CPU from ${U}/CPU/, a directory that does not exist, and asking for
that file at all is now known to be wrong: it records a core count, and
copying it between machines is exactly how the R740 came to be using two
of its twelve cores. Rewritten to ask for the one thing actually
outstanding, PAGENET_DLY.PCF, with an explanation of why it must be
copied rather than rebuilt. Timings replaced with measured ones (11 min
for EXAMPLE, ~2 h for a real PAGENET day) in place of an unsourced
"35 minutes". Sections describing the R740 and configuration versioning
are marked as now real, because they are.
gnss_automation_roadmap.md — not stale so much as orphaned. It carries
the same original date as project_documentation/roadmap.md; that one was
restructured, is maintained and is referenced five times, while this one
sat for six months with zero inbound links. Its detailed feature designs
have no equivalent elsewhere, so it is reclassified as a design backlog
rather than rewritten or deleted, with its stale phasing called out and
a pointer to the maintained roadmap. roadmap.md now links back, so it
stops being invisible.
Eleven findings, all confirmed against real data before being accepted. Two of them defeated guarantees this code advertises in its own docstrings. provision_opt_dir had no dry-run mode. provision_gpsuser.py printed "DRY RUN — nothing will be written" and then wrote every panel, because there was no parameter to ask it not to. Verified against PR #65's gold tree: the default invocation wrote all six PGN_WK panels into $U. Dryness is now a real parameter and the dry run writes zero files. The strict gate was also inverted and its refusal exited 0. strict=apply tied hazard checking to the write mode, so checks were OFF in dry run (which wrote) and ON only under --apply (which did not). Worse, a refusal was returned in the warnings list, so a run that provisioned nothing at all still exited 0 — the sixth instance in this project of a swallowed exit status reporting success. Strictness is now unconditional and refusals are errors; a refusing run exits 1. --stations silently reverted MAXPAR. compute_maxpar is max(1000, N*4+500), which for the ~72-station PAGENET network yields 1000 — overwriting the "10000" the real ADDNEQ2 panel carries, raised deliberately after an observed parameter overflow and flagged in PR #65 as not to be reverted. MAXPAR is a ceiling: it is now raised, never lowered. The existing test asserted the lowering behaviour and has been corrected. add_station_to_campaign.py took the station code from MARKER NAME — the exact bug fixed in the validator hours earlier on this same branch. Against a PAGENET header it derived name="BOGO", dome="PBOG" and would have written "BOGO PBOG" into five reference files. It now calls _resolve_station_code. The DOMES is additionally format-checked rather than taken as "whichever marker field is not the code", which produced "BOGO CITY" as a DOMES number; stations without one are written as a bare 4-char name, matching how PGN.STA already lists them. A partially-added station reported "nothing to do" and returned 0, leaving the campaign inconsistent and RXOBV3 still aborting. Partial adds now refuse loudly, and the presence test is anchored per line rather than a substring search that matched codes inside DOMES numbers. Added a hardcoded_campaign hazard class. PR #65's PROVENANCE.md listed MENU.INP under "expect the provisioner to catch these", but "${P}/SOB" — the instructor's demo campaign — matched no hazard class; the backslashes were auto-converted and the panel reported clean. Both MENU.INP and MENU_CMP.INP are now flagged. Two existing tests had been using that same broken line as their example of a CLEAN panel. require_stations now defaults to True. Fixing _is_rinex_obs removed the instance of the vacuous pass; leaving the safe behaviour opt-in left the failure mode. Production already passes it explicitly, so only direct callers change. One test conflated "this station has no data" with "there is no data at all" by proving the former with an empty directory; it now stages data for a different station. Also: verify_patrol_check.sh verified ~/patrol_check.sh rather than the repo copy this branch fixes, so it could have exercised a stale script and reported the old wrong numbers; inspect_member6.sh redirected as root into a predictable /tmp path open to a symlink attack, now mktemp with a trap; patrol_check.sh reported one arbitrary drive's power-on hours as an array-wide fact, now a cross-member range; and add_station's duplicated .Z reader leaked the child gzip, so it now reuses the validator's context manager instead of keeping a second copy that had already diverged. 194 tests pass, up from 189. ruff clean.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/sudo/inspect_member6.sh (1)
70-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequest the grown defect list explicitly.
smartctl -i -Hreads identity and health data, but it does not load the SCSI grown-defect list. This query cannot support the verdict condition on a growing grown-defect list. Usesmartctl -l defects -d megaraid,$id "$VD"or another supported all-information query for the targetsmartmontoolsversion, then ensure the generated output includesElements in grown defect list.🤖 Prompt for 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. In `@scripts/sudo/inspect_member6.sh` around lines 70 - 72, Update the smartctl query in the inspection block to explicitly request the SCSI grown-defect list, using the supported defects-list or equivalent all-information option with the existing megaraid device target. Preserve the current identity and health output while ensuring the filtered generated output includes “Elements in grown defect list” for the verdict logic.
♻️ Duplicate comments (1)
scripts/patrol_check.sh (1)
99-115:⚠️ Potential issue | 🟠 MajorFail closed when RAID evidence is unavailable.
Both scripts can produce successful-looking diagnostics from missing
smartctlevidence.
scripts/patrol_check.sh#L99-L115: reject unavailable fields, nonzeroUNCORR, and invalidSWEEPSvalues; return nonzero for any incomplete member.scripts/sudo/inspect_member6.sh#L71-L91: separate command failure from grep no-match and require identity, error, and background markers.scripts/sudo/inspect_member6.sh#L95-L100: do not convert a failed background query into an empty entries file and valid-looking samples.🤖 Prompt for 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. In `@scripts/patrol_check.sh` around lines 99 - 115, Make the RAID diagnostics fail closed across all listed sites: in scripts/patrol_check.sh lines 99-115, reject missing or invalid evidence, nonzero UNCORR, and invalid SWEEPS values, returning nonzero for any incomplete member; in scripts/sudo/inspect_member6.sh lines 71-91, distinguish command failures from grep no-match and require identity, error, and background markers; in scripts/sudo/inspect_member6.sh lines 95-100, preserve background-query failures instead of creating an empty entries file or valid-looking samples.
🧹 Nitpick comments (1)
docs/gnss_automation_roadmap.md (1)
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the warning blockquote continuous.
The blank line at Line 36 splits the blockquote and triggers markdownlint MD028. Remove the blank line or prefix it with
>so the warning renders as one block.🤖 Prompt for 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. In `@docs/gnss_automation_roadmap.md` around lines 34 - 36, Update the warning blockquote near “Treat everything below as a menu of designs, not a schedule” so it remains continuous: remove the blank line after it or prefix that line with “>”, preserving the warning as a single Markdown blockquote and satisfying MD028.Source: Linters/SAST tools
🤖 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 `@docs/bernese_orchestration_explainer.md`:
- Around line 78-80: Update docs/bernese_orchestration_explainer.md at lines
78-80 to qualify the repository claim so it excludes the uncaptured
PAGENET_DLY.PCF, or capture that asset before retaining the broader wording;
update lines 201-204 to replace the complete production-environment rebuild
guarantee with a statement limited to the currently version-controlled
provisioning assets.
- Around line 81-82: Update the summary table in
bernese_orchestration_explainer.md to scope the reproducibility claim to
version-controlled settings, explicitly distinguishing them from the per-run
outcome records that are not currently captured. Keep the surrounding
explanation consistent with this limitation.
In `@scripts/sudo/inspect_member6.sh`:
- Line 51: Update the directory setup and logging flow around mkdir and tee:
print the log path and exit successfully when tee fails, but propagate
directory-creation failures as errors. Capture the command-substitution/tee
status before the final echo, and apply the same handling to the corresponding
logic around lines 116-119.
- Around line 57-58: Make the mktemp assignment for ENTRIES fail the script
immediately when temporary-file creation fails, and only install the existing
cleanup trap after that successful assignment. Preserve the current EXIT cleanup
behavior for valid temporary paths.
In `@services/bernese-workflow/src/bernese_workflow/panel_sanitizer.py`:
- Around line 156-159: Update sanitize_panel_text so _QUOTED_RE searches
normalized new_line content rather than raw stripped text, while retaining
stripped as the PanelWarning context. Add a test covering a ${P}\SOB\... path
that verifies separator conversion and emits a hardcoded_campaign warning. Apply
these changes in
services/bernese-workflow/src/bernese_workflow/panel_sanitizer.py lines 156-159
and services/bernese-workflow/tests/test_panel_sanitizer.py lines 257-274.
---
Outside diff comments:
In `@scripts/sudo/inspect_member6.sh`:
- Around line 70-72: Update the smartctl query in the inspection block to
explicitly request the SCSI grown-defect list, using the supported defects-list
or equivalent all-information option with the existing megaraid device target.
Preserve the current identity and health output while ensuring the filtered
generated output includes “Elements in grown defect list” for the verdict logic.
---
Duplicate comments:
In `@scripts/patrol_check.sh`:
- Around line 99-115: Make the RAID diagnostics fail closed across all listed
sites: in scripts/patrol_check.sh lines 99-115, reject missing or invalid
evidence, nonzero UNCORR, and invalid SWEEPS values, returning nonzero for any
incomplete member; in scripts/sudo/inspect_member6.sh lines 71-91, distinguish
command failures from grep no-match and require identity, error, and background
markers; in scripts/sudo/inspect_member6.sh lines 95-100, preserve
background-query failures instead of creating an empty entries file or
valid-looking samples.
---
Nitpick comments:
In `@docs/gnss_automation_roadmap.md`:
- Around line 34-36: Update the warning blockquote near “Treat everything below
as a menu of designs, not a schedule” so it remains continuous: remove the blank
line after it or prefix that line with “>”, preserving the warning as a single
Markdown blockquote and satisfying MD028.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2d7a269-ac20-4e06-aa68-8f1dbfef4e54
📒 Files selected for processing (13)
docs/bernese_orchestration_explainer.mddocs/gnss_automation_roadmap.mddocs/project_documentation/deliverables_tracker.mddocs/project_documentation/roadmap.mdscripts/add_station_to_campaign.pyscripts/patrol_check.shscripts/provision_gpsuser.pyscripts/sudo/inspect_member6.shscripts/sudo/verify_patrol_check.shservices/bernese-workflow/src/bernese_workflow/panel_sanitizer.pyservices/bernese-workflow/src/bernese_workflow/rinex_header_validator.pyservices/bernese-workflow/tests/test_panel_sanitizer.pyservices/bernese-workflow/tests/test_rinex_header_validator.py
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/sudo/verify_patrol_check.sh
- services/bernese-workflow/src/bernese_workflow/rinex_header_validator.py
- scripts/provision_gpsuser.py
| echo "ERROR: needs root — run with sudo." >&2 | ||
| exit 1 | ||
| fi | ||
| mkdir -p "$LOGDIR" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list =="
git ls-files | grep -E '(^|/)scripts/sudo/inspect_member6\.sh$' || true
echo "== relevant file outline/size =="
wc -l scripts/sudo/inspect_member6.sh
sed -n '1,150p' scripts/sudo/inspect_member6.sh
echo "== references to LOGDIR/LOG/mkdir/tee/exit =="
rg -n 'LOGDIR|LOG|mkdir -p|tee|exit|[0-9]+:' scripts/sudo/inspect_member6.shRepository: alfieprojectsdev/movefaults
Length of output: 5892
Propagate directory and log-write failures.
If mkdir -p "$LOGDIR" or tee "$LOG" fails, print the log path and exit successfully. Error out when directory creation fails, and capture the command-substitution/tee status before the final echo.
Also applies to lines 116-119.
🤖 Prompt for 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.
In `@scripts/sudo/inspect_member6.sh` at line 51, Update the directory setup and
logging flow around mkdir and tee: print the log path and exit successfully when
tee fails, but propagate directory-creation failures as errors. Capture the
command-substitution/tee status before the final echo, and apply the same
handling to the corresponding logic around lines 116-119.
Comment on lines
+57
to
+58
| ENTRIES="$(mktemp)" | ||
| trap 'rm -f "$ENTRIES"' EXIT |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)inspect_member6\.sh$|scripts/sудо/inspect_member6\.sh$' || true
echo "== file excerpt =="
if git ls-files | rg -q '(^|/)inspect_member6\.sh$'; then
file="$(git ls-files | rg '(^|/)inspect_member6\.sh$' | head -n 1)"
echo "file=$file"
wc -l "$file"
sed -n '1,140p' "$file" | nl -ba
fi
echo "== shell options and mktemp usage =="
if git ls-files | rg -q '(^|/)inspect_member6\.sh$'; then
file="$(git ls-files | rg '(^|/)inspect_member6\.sh$' | head -n 1)"
rg -n 'set -[eEuo]|mktemp|trap|ENTRIES' "$file"
fi
echo "== deterministic simulation: what later redirections do when mktemp output is empty =="
python3 - <<'PY'
import shlex
lines = [
r'ENTRIES="$(mktemp)"',
"trap 'rm -f \"$ENTRIES\"' EXIT",
"printf '%s\\n' 'identity' > \"$ENTRIES\"",
]
for line in lines:
print(f"line={line}")
# This only models shell word splitting/expansion; mktemp output is the only dynamic path.
PYRepository: alfieprojectsdev/movefaults
Length of output: 348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="scripts/sudo/inspect_member6.sh"
echo "== file first 150 lines =="
awk '{printf "%6d\t%s\n", NR, $0}' "$file" | sed -n '1,150p'
echo "== mktemp/trap/option usage =="
grep -n -E 'set -[eEuo]|set --|mktemp|trap|ENTRIES|cat|>' "$file" || true
echo "== simulate invalid ENTROPY behavior for later redirects =="
python3 - <<'PY'
path = ""
try:
with open(path, "a") as f: # same path is used by later redirects in the snippet
f.write("identity\n")
except Exception as e:
print(type(e).__name__, str(e))
PYRepository: alfieprojectsdev/movefaults
Length of output: 7694
Exit when mktemp fails.
The script only enables set -uo pipefail; if mktemp cannot create the entries file, ENTRIES is empty and later writes such as > "$ENTRIES" fail silently before tee "$LOG". Make mktemp required before installing the cleanup trap.
Proposed fix
-ENTRIES="$(mktemp)"
+ENTRIES="$(mktemp)" || {
+ echo "ERROR: cannot create temporary entries file." >&2
+ exit 1
+}
trap 'rm -f "$ENTRIES"' EXIT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ENTRIES="$(mktemp)" | |
| trap 'rm -f "$ENTRIES"' EXIT | |
| ENTRIES="$(mktemp)" || { | |
| echo "ERROR: cannot create temporary entries file." >&2 | |
| exit 1 | |
| } | |
| trap 'rm -f "$ENTRIES"' EXIT |
🤖 Prompt for 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.
In `@scripts/sudo/inspect_member6.sh` around lines 57 - 58, Make the mktemp
assignment for ENTRIES fail the script immediately when temporary-file creation
fails, and only install the existing cleanup trap after that successful
assignment. Preserve the current EXIT cleanup behavior for valid temporary
paths.
Moves README_FOR_GPS3_CLAUDE.md into the repo. It is the T420 session's instructions for the DOSTB20150918 external drive, written immediately before the drive was unplugged and carried to the R740: what is on it, what must not be touched, and why it must be mounted read-only. The operative point is that the drive being on this machine's USB bus removes the 6 MB/s wifi bottleneck that has kept /srv/gnss-archive/legacy empty. 26 GB of RECOVERED_* is minutes rather than a day. That is the highest-value thing available while it is attached, and it is the only irreversible one — the drive is currently the sole complete copy of material rescued from three dead or dying disks. Two corrections to the session log, both found by the T420 rather than by me: Section 15.4 claimed "all work reached main". It had not and has not. origin/main is still at 1d1082e (PR #60); PRs #61-#65 are all open. Rule 5 says to verify origin/main actually advanced rather than infer it from a successful push, and this was written in the section meant to be the authoritative end-of-session state. Section 14.5 predicted that PAGENET_DLY.PCF, being RNX2SNX truncated at 514, would leave 599 DUMMY waiting on an undefined 522. Now that the real file is available, that is wrong: its 599 waits on 512 514, there is no 521/522, and find_dangling_waits reports zero. The reduction was done properly. The advice to capture rather than re-derive survives, but on the weaker and more honest ground that the captured file is the one actually validated and the 9xx tail involves choices a reconstruction would guess at. One further note on method. The script applying these two corrections printed "both corrections applied" unconditionally, and the second replacement had silently failed to match on a backtick. A success message from something that verified nothing — the same defect the log catalogues five times in section 15.5, committed while correcting an entry about it. Both edits are now confirmed present by grep.
The handover's RECOVERED_* table listed three directories totalling 26 GB and stated that this was "not the ~157 GB the continuity audit refers to". The user identified a fourth, RECOVERED_SEAGATE_W2A0W9T2_DATA0, absent from the table entirely. The arithmetic supports him: 157 G measured on the drive (RESUME_NEXT.md, "RAW alone 125 G") minus the 26 G listed leaves ~131 G, close to the raw-observation figure the audit calls out separately. The omitted directory is therefore probably the bulk raw data — the most valuable and least reproducible material on the drive, and the one directory left out of the transfer plan. The omission happened because three names were written out by hand. Both scripts here glob RECOVERED_* and report what they find, so a fifth directory would be copied rather than silently skipped. A typed list cannot notice what it omits. mount_dostb.sh mounts read-only, resolving the device by filesystem label rather than /dev/sdX, which is not stable across reboots or other USB devices on this machine. Read-only matters because the drive is currently the only complete copy of material rescued from three dead disks, and /srv/gnss-archive/legacy is still empty. The mount is proved read-only by attempting a write and requiring it to fail, rather than by trusting the option string. It refuses to unmount while files are open, since pulling the mount from under a running rsync yields a partial copy that still exits 0. It also declines to suggest ntfsfix or remove_hiberfile on a dirty NTFS volume: both write to the drive. archive_transfer.sh censuses files, symlinks, directories and bytes independently on both sides and compares them per directory. rsync's exit code is recorded but decides nothing — it exits 0 having skipped unreadable files and 23 on a run that copied 99.99% successfully. Zero symlinks on the source is expected, the drive being NTFS, and is called out so it is not chased as a fault. It refuses to run while a headless BPE is active, since concurrent bulk I/O hangs one, and excludes its own command line from that check because pgrep -f matching itself has produced false readings here three times. On success it explicitly declines to report "the archive is backed up", directing instead to the sha256 manifest and to reporting per directory. Sizes in the corrected README remain inferred by subtraction, not measured; the census supersedes them once the drive is attached.
…157 GB
Run at the user's request before the transfer, and it found the one bug
that would have mattered.
rc_worst was assigned inside a subshell. The whole body is
`{ ... } 2>&1 | tee "$LOG"`, which is a pipeline, so every assignment
happened in a child and the parent's `exit "${rc_worst:-0}"` read an
unset variable. The script returned 0 for every run — including one that
had just printed "*** MISMATCH ***" after comparing censuses. The
verification existed, printed correctly, and then reported success
regardless.
That is the seventh instance of a swallowed exit status in this project,
and it was in the script written specifically because rsync's exit code
cannot be trusted. Fixed by exiting from inside the block and reading
${PIPESTATUS[0]}, the same discipline already used in
verify_patrol_check.sh. Verified against a deliberate mismatch: the old
pattern exits 0, the new one exits 1.
Also fixed: the destination census fields were read into `df dl dd do
db`, where `do` is a bash keyword (SC1010). `bash -n` had accepted it.
Both censuses now read into arrays and index the two fields actually
compared, which removes six unused-variable warnings; `cd` as a variable
name is gone too, since it shadows the builtin. Dropped two leftover
unused accumulators.
One self-inflicted round trip worth recording: the first fix left a
comment beginning with the linter's own name, which is parsed as a
directive and produced three new errors. A comment explaining a lint fix
cannot start with the linter's name.
All five scripts under scripts/ are now shellcheck-clean.
alfieprojectsdev
added a commit
that referenced
this pull request
Aug 4, 2026
Both provenance documents understated the hazards they exist to record, and both did so in the direction of false reassurance. gpsuser52-luzon/PROVENANCE.md said "Six live .INP files carry C:\Bernese\... absolute paths" and listed six. Measured by running sanitize_panel_text over all 105 live panels in that tree: 50 carry foreign absolute paths across 200 lines, and 72 of 105 carry a hazard of some kind (820 hardcoded_campaign, 200 foreign_abs_path, 95 hardcoded_date). An eight-fold understatement in the hazard document is worse than no document, because someone remediating six panels stops after six and believes the tree is clean. All 50 are now listed, with the command to reproduce the census. gpsuser/PROVENANCE.md listed MENU.INP under "expect the provisioner to catch these". It did not. sanitize_panel_text returned changed=True, warnings=0 for that file: the backslashes were converted and the panel reported clean, because the hardcoded campaign SOB matched no hazard class. A hardcoded_campaign class was added in PR #64 to close it, and MENU_CMP.INP — absent from the list entirely — turns out to carry the same defect. Merges origin/main into this branch, which is what made the second finding verifiable: checked on the branch alone, the panels reported clean, because the branch predated the sanitizer that detects the hazard. Verifying a claim about tooling against the wrong version of that tooling produces exactly the reassurance the claim was supposed to justify. Verified on the merged tree: MENU.INP and MENU_CMP.INP raise 2 hardcoded_campaign warnings each, ADDNEQ2.INP raises 20 across three classes, provision_gpsuser.py --apply refuses the tree and exits 1, and the 194 bernese-workflow tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Session log §13–14 covering 2026-07-30 → 08-03, plus the tooling and doc
corrections behind them. Docs and scripts only — no application code changes.
Surface scanning: the §12.4 gap is closed
§12.4 left the log asserting that possibly nothing performs full-surface
reads of the 16 RAID 5 members, with the fix gated behind an iDRAC that has no
IP. The premise was too narrow. Scanning need not come from the controller:
BMS, the drive firmware's own Background Media Scan, runs below the PERC and
is readable through the megaraid pass-through.
i.e. a full pass roughly daily.
repaired in place while parity was still intact — nothing reassigned.
count × capacity vs lifetime bytes read, matching within 0.8%.
SMART long tests are rejected as duplicative; the §9 smartd config is
unchanged. Whether PERC patrol read is also enabled remains unknown and no
longer matters.
Member 6 investigated and cleared
At 671 recovered sectors — seven times its nearest peer — member 6 looked like
a candidate for proactive replacement. It is healthy. These are 512e drives, so
one 4K physical sector logs as eight 512-byte entries: 671/8 ≈ 84, matching
that drive's own corrected-error counter exactly. Defects span power-on hours
121–10,127 at a flat rate rather than clustering recently, and nothing was
reassigned.
The useful part is the inversion: member 0, with zero BMS entries, carries
228 corrected read errors and 3,929 correction invocations against
member 6's 84 and 85. Ranking drives by defect count alone selects the wrong
suspect.
Bernese R740 deployment
USER.CPUmaxjobs was2— the T420's value, carried across with theconfig. The R740 was using 2 of its 12 cores against the known 40-min
502 GPSCLU_P bottleneck. Now 11, set via
cpu_config.compute_maxjobs(12, ram_gb=62, reserve_cores=1)rather than byhand, so it is the code path production will use.
a 12-core Xeon Silver 4214R (24 threads). Since maxjobs tracks physical cores
for FPU-bound sub-solves, 24 would have oversubscribed by 2× and plausibly run
slower than a correct 12. Corrected in place.
objcopypatch that doc flagged as uncertainis not needed.
both), and that all acceptance-test inputs are already on the box (677 PAGENET
RINEX, DOY 081–090; full
PGN.*reference set).New P0 blocker — task C2
validate_rinex_headers()finds zero RINEX in the real DATAPOOL._is_rinex_obs()matches onpath.suffixagainst.rnx/.obs/.rxo/.<yy>o, butevery file there is gzipped; decompressed they are Hatanaka
.26dor.crx,neither accepted.
It failed loudly only because the call passed
require_stations=True. Underthe default it returns a passing report having inspected nothing — the
vacuous pass its own docstring warns about, reached through an unanticipated
door. All 128 tests pass, because the fixtures use uncompressed names, so
the suite cannot see this class of failure.
Tooling and runbook
patrol_check.shmoved into the repo with two defects fixed: the SCSIerror-counter columns were transposed (reporting 0 TB read and 1,025,339
uncorrected errors — both exactly backwards), and the recovered column now
reports physical sectors.
scripts/sudo/, logs gitignored andscripts committed, so sudo steps need no copy-paste between terminals.
docs/gps3_tmux_claude_runbook.md— running Claude Code under tmux ongps3, written for someone who has never used tmux. Start tmux first (a
running process cannot be moved in later); a reboot destroys every session
while cron and smartd carry on regardless.
Verification
patrol_check.shre-run after the fix: SWEEPS 1.01x, UNCORR 0 on all 16.uv run pytest services/bernese-workflow/tests/— 128 passed.fsck cleaneach run.Second commit — C2: the validator was blind to every file in a real DATAPOOL
All seven PAGENET sessions (DOY 084–090) now validate clean, from zero files
visible. 179 tests pass, up from 128.
The defect
_is_rinex_obs()matched onpath.suffix. Every file in the gps3 archive iscompressed — 3,010
.gz, 20.Z— sosuffixwas always the compressionextension. Hatanaka was an independent second miss:
.26d/.crxwere absentfrom the accepted set, so even decompressed most PAGENET files would have been
skipped.
The consequence was worse than skipped files. An empty scan is an error only
when
require_stations=True; under the default the function returned apassing report having examined nothing — approving every session while
inspecting none of it, with the first symptom arriving much later as an RXOBV3
hard abort mid-BPE.
All 128 tests passed throughout, because the fixtures used uncompressed
.YYo/.rnxnames that do not occur in the data.The fix was smaller than expected
No Hatanaka decoding is needed. A CRINEX file stores the original RINEX
header verbatim after two
CRINEX VERS/PROGlines, compacting only therecords below
END OF HEADER— which this validator never reads.crx2rnxnever runs: no RNXCMP build, no
hatanakapackage.Compression needed two paths. IGS convention is
.Z(UNIX compress/LZW),and Python's
gzipcannot read it —BadGzipFile, magic1f 9dagainst gzip's1f 8b. Pythongziphandles.gz, keeping the 3,010-file pathsubprocess-free; GNU
gzip -dchandles the 20.Z. No new dependency. Suffixstripping loops right-to-left, since every real name stacks two extensions and
both orders occur.
Two more defects, visible only once the scan returned anything
Descriptive marker names shadowed the station code. PAGENET CORS write
MARKER NAME = "BOGO CITY"with the code inMARKER NUMBER = "PBOG"; IGSfiducials do the reverse (
CUSV/ 9-char DOMES). TakingMARKER NAME[:4]gaveBOGO, absent fromPGN.STA— 9 of 72 stations failed on data that processescorrectly. A validator that fails on good data gets switched off.
rglobdescended into the hand-quarantined.excluded_plg2/, reportingPLG2 missing on exactly DOY 086 and 088 — independently reproducing readiness
§2.2's empirical finding. But RNX_COP globs without recursing, so those files
can never be staged. The validator must model what will actually be processed;
flagging unreachable files is the mirror image of the vacuous pass.
Test coverage
Now spans the production filename space, which is precisely what the previous
128 did not:
(nav/met/SP3/CLK/BIA must not be mistaken for observations)
.gz/.Z.Zis testable with no externalcompressor — nothing on stock Ubuntu can write
.Z, and the real samplesare 1.6 MB. Verified round-tripping through GNU
gzip.gzipisnot treated as failure
Verification
Pre-existing collection errors in
drive-archaeologist/vadase-rt-monitor/field-ops(missing optional deps) are untouched and unrelated.ruff formatis not enforced repo-wide (59 of 92 files would reformat), so no reformatting
was applied.
Summary by CodeRabbit