feat(field-ops): deployable PWA — offline photo fix, login, R2 storage, fail-closed prod gate - #68
feat(field-ops): deployable PWA — offline photo fix, login, R2 storage, fail-closed prod gate#68alfieprojectsdev wants to merge 3 commits into
Conversation
…k theme
The logsheet PWA had no stylesheet at all -- 31 inline style objects and
nothing else. Fine for proving the form logic, wrong for showing MOVE Faults
staff what they would actually carry into the field.
Base is Pico.css (classless). It styles semantic <label>/<input>/<select>
directly, so the JSX stays readable and the diff stays small: the shared
inputStyle/readonlyStyle constants are now empty rather than deleted, which
leaves the ~20 `style={inputStyle}` call sites untouched and gives one obvious
place to reintroduce an override if a single control ever needs one.
On top of that, src/styles/field.css -- the layer Pico does not provide. Every
rule is justified by how this app is used: outdoors, on a phone, by someone at
a GNSS monument who may be wearing gloves and squinting into tropical sun.
48px tap targets gloves, and a moving vehicle or boat
16px input font below this iOS zooms the viewport on focus,
which is disorienting one-handed
readonly fields at 1.25rem avg slant and RINEX height are DERIVED, not typed,
and are the numbers that silently corrupt the
vertical component if wrong -- they should not
look like something you can edit
prefers-contrast: more screens wash out badly in sunlight; honour the OS
signal rather than guessing
single column below 26rem paired time inputs become unusable on a small
phone in portrait
Theme support is three-state -- system / light / dark -- not a two-way toggle.
Phones switch on schedule or ambient light, which is what you want when a
session starts at 05:00 and ends under midday sun, so "system" follows the OS
and keeps following it. But an operator who has decided the screen is
unreadable needs to pin it and have that stick, so an explicit choice persists
and overrides.
Two details that are load-bearing rather than decorative:
Cascade order. :root, then @media (prefers-color-scheme: dark) scoped to
:not([data-theme="light"]), then explicit [data-theme] last. That ordering is
what lets a pinned theme beat the OS in BOTH directions -- pinning light on a
dark phone works, not just the reverse. Verified in the built bundle by byte
offset, not assumed.
Pre-paint script in index.html. Without it the page renders light for one
frame and then flips: a white flash into the eyes of someone working at dawn
or at night, which is precisely the user dark mode exists for. It is inline
and dependency-free so it runs before the bundle; useTheme takes over once
React mounts, and both update <meta name="theme-color"> so the PWA's browser
chrome matches instead of staying blue.
All seven custom colours are declared once per theme as tokens. They were
hardcoded light values and would have looked wrong the moment dark engaged.
Dark variants use lifted lightness and lower saturation because saturated
colour on dark haloes on OLED, which is most field hardware.
Cost: 12.60 kB gzipped CSS total. Typecheck and production build clean.
Also: node_modules/ was not gitignored and package-lock.json was untracked --
for a service whose whole premise is field reliability, an unpinned dependency
tree is a real hazard. Both fixed here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… screen Three defects that would have caused data loss during the Palawan fieldwork, found by auditing the app against real field use rather than by testing it on a desk with signal. 1. Offline submissions destroyed the mandatory photo. The offline path queued `record`, which is LogSheetIn and has no photo field, then called reset() -- which clears the file input. The blob was gone from the browser entirely, while the UI reported "Saved offline. Will sync automatically when connected." That sentence was true of the text and false of the photo, and there was no recovery path: the Queue view was a stub, so nothing listed what was pending or let anyone re-attach. A photo is REQUIRED to submit. So the app enforced a photo and then silently discarded it on the exact path field staff use most -- offline is the normal case in Palawan, not the exception. The blob is now stored in IndexedDB with the record (schema v1 -> v2, an additive upgrade that carries existing pending records forward rather than recreating the store, since dropping it would discard unsynced fieldwork) and uploaded after the logsheet POST succeeds. Exactly-once under retry: the logsheet POST is idempotent server-side (ON CONFLICT client_uuid DO NOTHING, then re-fetch), but photo upload has no such guard. `_photoUploaded` is persisted BEFORE the record is marked synced, so a crash between the two leaves it pending with the photo flagged done; the next flush re-POSTs the logsheet harmlessly and skips the photo. Verified against the running API: the same client_uuid posted twice returns the same id and leaves one row. A record is marked synced only once both halves are on the server. If the photo fails, it stays pending and retries -- the blob is never dropped. 2. There was no way to log in. login() has existed in services/api.ts since the service was scaffolded, but nothing called it and no form existed. The only way to use the app was to mint a JWT by hand and write it into localStorage, which is not something a field observer can do on a phone at a monument. Adds LoginScreen and an auth gate. Offline is distinguished from a wrong password, because one is retryable where you stand and the other is not. 3. Nothing showed what was queued. The Queue tab now lists every record with its photo state and age, shows device storage headroom, and offers a manual sync -- signal often returns as a brief window and waiting for the browser's own `online` event is not always fast enough to catch it. An operator can confirm the day's work exists before leaving a site, which "records will sync automatically" with an empty list could not. Also: a storage-quota guard before queueing (phone photos are ~3 MB and a QuotaExceededError on the offline path would look exactly like a successful save), and an offline banner in the header, since connectivity is the single most important thing to know in the field and should not have to be inferred from a failed submit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y config
Prepares the logsheet PWA for a temporary deployment supporting the Palawan
fieldwork: Vercel for the PWA, a container host for the API, Neon for Postgres,
Cloudflare R2 for photos (per docs/asset-storage-cloudflare-cloudinary-neondb.md).
Photos no longer go to local disk. write_bytes() to a path under /tmp is correct
on a workstation and wrong on every deployment target: a container filesystem is
ephemeral, so the row in logsheet_photos would keep pointing at a file the next
restart had already destroyed -- the database would look healthy and the
evidence would be gone. storage.py adds a configured backend, local for dev and
R2 for deployment, and the bytes are written BEFORE the row so a failed upload
fails the request rather than committing a reference to an object that was never
stored.
THE STARTUP GATE IS THE IMPORTANT PART OF THIS COMMIT.
FIELD_OPS_PRODUCTION=1 refuses to boot if the JWT secret is the shipped default
or under 32 chars, if storage is not R2, or if DATABASE_URL is unset. This repo
is PUBLIC: with the default secret, anyone who reads it can mint a valid token
for the deployed URL and post logsheets as any user. A service that boots
happily in that state is worse than one that will not boot, because nobody finds
out. Verified by running it -- all conditions fire together, correct config
boots, dev mode is unaffected, and the message names variables without ever
printing a value.
The R2 backend fails closed the same way: selecting it without complete
credentials raises at startup, not at the first upload after someone has already
left the site. Also verified by execution, including the unknown-backend case.
Three things that would have broken the deploy on arrival:
- port=8001 was hardcoded. Container hosts assign PORT at runtime and route to
it, so the health check never passes and the deploy rolls back. Now read
from the environment.
- reload=True was unconditional. In a container that wastes memory on a file
watcher and can restart the process mid-request. Now opt-in via
FIELD_OPS_DEV=1.
- CORS was hardcoded to localhost. Now extensible via FIELD_OPS_CORS_ORIGINS,
with "*" deliberately unsupported -- browsers reject it alongside
allow_credentials anyway, and an open policy on a service holding field data
should not be reachable by accident.
DATABASE_URL support for hosted Postgres, which needs two fixups that are
otherwise puzzling to debug: the driver must be named explicitly or SQLAlchemy
loads psycopg2 and fails at import, and asyncpg rejects libpq's ?sslmode= that
Neon includes in its copy-paste string.
The frontend API base is configurable but defaults to same-origin relative
paths, which is what both the Vite dev proxy and a Vercel rewrite expect. Same
origin means no CORS preflight -- one fewer round trip on a connection that may
be a single bar of signal.
DEPLOY.md is the runbook. Every step requiring an account or a credential is
marked [you]; no secret values appear in this repo, only names. Section 6 is a
verification checklist to run on a real phone before anyone travels, and its
airplane-mode step is the one that matters: it exercises the offline path that
the previous commit fixed, whose failure mode is silent.
Known gaps are listed rather than left to be discovered: no frontend tests
(including none for the slant->RH computation), no password reset, no admin
view, and free-tier cold starts.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds production-ready photo storage, offline photo synchronization, authentication, theme controls, responsive styling, configurable API routing, deployment configuration, and Field Ops operating documentation. ChangesField Ops application
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant LogSheetForm
participant IndexedDB
participant BackendAPI
participant PhotoStorage
Operator->>LogSheetForm: submit log record and photo
LogSheetForm->>IndexedDB: queue record and photo when offline or upload fails
LogSheetForm->>BackendAPI: submit record when online
BackendAPI->>PhotoStorage: save photo
PhotoStorage-->>BackendAPI: return storage reference
BackendAPI-->>LogSheetForm: return submission result
LogSheetForm->>IndexedDB: synchronize pending records and photos
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/field-ops/frontend/src/components/LogSheetForm.tsx (1)
291-305: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
addToQueuein the photo-failure branch.
addToQueuethrowsQueueStorageErrorwhen device storage is short. The offline path at lines 274-283 and the network-failure path at lines 311-320 both catch it. This branch does not. The rejection escapesonSubmit,submitStatestays"saving", and the submit button stays disabled. The operator then reads "Saving…" with no way to retry, while the photo exists only in the form.🐛 Proposed fix
} catch { // Logsheet is on the server but the photo is not. Queue the photo // rather than asking the operator to remember to re-attach it later: // by then they have left the site. _photoUploaded stays false, so the // next flush re-POSTs the (idempotent) logsheet and retries the photo. - await addToQueue(record, photo); - setSubmitState("queued"); - setErrorMsg("Log saved. Photo queued — it will upload on the next sync."); - reset(); - return; + try { + await addToQueue(record, photo); + setSubmitState("saved"); + setErrorMsg("Log saved. Photo queued — it will upload on the next sync."); + reset(); + } catch (queueErr) { + // Device storage is full: keep the form and the photo so the + // operator can free space and retry the photo. + setSubmitState("error"); + setErrorMsg( + queueErr instanceof Error + ? `Log saved, but the photo could not be queued. ${queueErr.message}` + : "Log saved, but the photo could not be queued." + ); + } + return; }This also sets
"saved", which is the state whose render branch displayserrorMsg. See the related comment on the status messages.🤖 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/field-ops/frontend/src/components/LogSheetForm.tsx` around lines 291 - 305, Guard the addToQueue call in the photo-upload failure branch of onSubmit with QueueStorageError handling, matching the existing offline and network-failure paths. On queue-storage failure, set submitState to "saved" and surface the error through errorMsg so the form is no longer stuck in "saving" and the photo remains available for retry.
🧹 Nitpick comments (3)
services/field-ops/src/field_ops/main.py (1)
32-36: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider gating the localhost origins on development mode.
The two localhost origins are always allowed, including on an internet-reachable deployment with
allow_credentialsenabled. A page served fromhttp://localhost:5173on an operator's machine can then send credentialed requests to the production API. The risk is small, and removing the origins in production tightens the surface at no cost.
settings.is_productionalready exists inservices/field-ops/src/field_ops/config.py, so the condition can reuse it.♻️ Proposed refactor
-_ALLOWED_ORIGINS = ["http://localhost:5173", "http://localhost:3000"] + [ +_DEV_ORIGINS = [] if settings.is_production else ["http://localhost:5173", "http://localhost:3000"] + +_ALLOWED_ORIGINS = _DEV_ORIGINS + [ o.strip() for o in (settings.field_ops_cors_origins or "").split(",") if o.strip() and o.strip() != "*" ]🤖 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/field-ops/src/field_ops/main.py` around lines 32 - 36, Update the _ALLOWED_ORIGINS construction to include the localhost origins only when settings.is_production is false; preserve the configured non-wildcard CORS origins in all environments.services/field-ops/frontend/src/components/LogSheetForm.tsx (1)
109-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the empty style objects.
inputStyleandreadonlyStylenow hold no properties. The comment explains the intent, and there is no runtime cost, so this is optional. Removing them and thestyle={inputStyle}attributes makes the styling source unambiguous: everything comes fromsrc/styles/field.css. A future reader will otherwise look for overrides that do not exist.🤖 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/field-ops/frontend/src/components/LogSheetForm.tsx` around lines 109 - 117, Remove the empty inputStyle and readonlyStyle declarations from LogSheetForm, then remove their corresponding style attributes from the affected controls. Preserve the existing field.css selectors and all form behavior so styling comes exclusively from the stylesheet.services/field-ops/src/field_ops/storage.py (1)
80-86: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet explicit timeouts and a retry policy on the R2 client.
R2Storageconstructs the S3 client without abotocore.config.Config, so it uses botocore defaults: 60 s connect/read timeouts and retries that can multiply upload latency. Pass an explicit config with bounded timeouts and a small retry cap so failed uploads fail quickly and the device can retry from its queue.🤖 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/field-ops/src/field_ops/storage.py` around lines 80 - 86, Update the R2 client निर्माण in R2Storage’s boto3.client call to pass an explicit botocore.config.Config instead of relying on defaults. Set bounded connect/read timeouts and a small retry cap so uploads fail fast and can be retried by the device queue, while keeping the existing endpoint_url, credentials, and region_name behavior unchanged.
🤖 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/field-ops/DEPLOY.md`:
- Line 6: Update the opening fenced code block in DEPLOY.md to specify the text
language by changing the fence opener to ```text, resolving the MD040
markdownlint violation.
- Around line 231-232: Update the deployment documentation covering photo
storage, database/photo exports, and the people-visible photo handling to define
a retention period and deletion procedure for both R2 objects and
field_ops.logsheet_photos. Specify that exported archives are encrypted,
access-controlled, and securely cleaned up after use before field deployment,
replacing the current “never deleted” guidance.
- Line 56: Update the deployment instructions around the DATABASE_URL example
and the fly secrets set usage to avoid putting credentials in shell commands or
arguments. Use Fly’s protected secret-import/stdin flow, such as fly secrets
import, with DATABASE_URL, R2_ACCESS_KEY_ID, and R2_SECRET_ACCESS_KEY supplied
through protected environment variables or standard input.
- Around line 220-223: Add frontend test-runner coverage for the campaign submit
calculation that produces avg_slant_m and rinex_height_m. Add TDD-style tests
covering every supported antenna model, normal and boundary slant values, plus
invalid and partial inputs, and ensure the tests assert the submitted values.
Configure the frontend package to run these tests before field deployment.
- Around line 169-170: Update the password-generation command in the deployment
instructions to invoke the project interpreter via uv run python, ensuring the
declared field-ops dependencies such as bcrypt are available.
- Line 242: Replace the destructive rclone sync command in the archive
instructions with a non-destructive copy workflow, using rclone copy to a dated
destination directory. Include a --dry-run preview before the actual copy and
preserve the existing R2 source and archive context.
In `@services/field-ops/frontend/src/App.tsx`:
- Around line 23-33: Ensure App’s authed state stays synchronized when
clearToken is called by apiFetch or uploadLogSheetPhoto, using a shared
authentication store or notification mechanism rather than only initializing
from getToken at mount. Update the affected authentication flow so a 401-driven
token clear causes App to render the login screen, and add a test covering that
behavior.
In `@services/field-ops/frontend/src/components/LogSheetForm.tsx`:
- Around line 652-662: Update the LogSheetForm message rendering so only one
photo-requirement message appears when no photo is selected. Preserve the
offline note as the sole message while offline, and show the red “Add a photo to
submit” error only in the appropriate post-interaction or online state rather
than on initial render.
- Around line 674-684: Update the submit branch that handles an online log save
with a photo failure to use submitState "saved" instead of "queued", preserving
its errorMsg text about the photo being queued. Adjust the render conditions in
LogSheetForm so errorMsg is displayed for the saved outcome, while the queued
message remains reserved for genuinely offline saves.
In `@services/field-ops/frontend/src/components/QueueView.tsx`:
- Around line 70-79: Update QueueView’s connectivity handling to use the
existing useOnline hook, importing it and deriving the button’s disabled state
and label from its reactive online value instead of directly reading
navigator.onLine. Preserve the busy-state behavior and manual sync functionality
during brief online windows.
In `@services/field-ops/frontend/src/hooks/useOfflineQueue.ts`:
- Around line 218-227: Update the module-level flush coordination used by
useOfflineQueue and flushQueue so concurrent hook instances share one in-flight
promise instead of starting separate uploads; create the promise wrapper around
the existing flush work, clear the guard in finally after await refreshCount(),
and return the shared promise. Also address the multi-instance pendingCount
staleness by sharing queue state through the module’s context or external-store
mechanism so addToQueue refreshes are visible to App.tsx and other consumers.
In `@services/field-ops/frontend/src/styles/field.css`:
- Line 126: Add an empty line before width: 48px at
services/field-ops/frontend/src/styles/field.css lines 126-126, and before
width: auto at lines 269-269, separating each declaration from the preceding
custom-property declarations to satisfy Stylelint.
In `@services/field-ops/frontend/vercel.json`:
- Around line 8-9: Add a CI or pre-deploy validation for the frontend Vercel
configuration that searches the API rewrite destination in vercel.json for
REPLACE-WITH-BACKEND-HOST and fails before release when the placeholder remains;
preserve the existing manual substitution flow and valid backend rewrite
behavior.
In `@services/field-ops/src/field_ops/config.py`:
- Around line 115-122: Update the production validation in the
settings/configuration flow to require non-empty r2_account_id,
r2_access_key_id, r2_secret_access_key, and r2_bucket whenever
field_ops_storage_backend is "r2"; append each missing credential to the
existing problems list so startup fails before serving traffic, while preserving
the current backend and DATABASE_URL checks.
- Around line 86-88: Update the is_production property to normalize
field_ops_production by trimming whitespace and applying case normalization,
then treat the accepted truthy values (including "1" and "true") as production.
Ensure invalid or unrecognized values do not disable production checks in
_assert_deployable.
- Around line 69-79: Update the database URL normalization flow to preserve TLS
semantics when removing sslmode parameters: capture the original sslmode value,
strip it from the URL, and propagate required or certificate-verifying modes
through the create_async_engine connection configuration using an appropriate
asyncpg SSL context. Ensure TLS-required configurations are not downgraded to
plaintext, and log the stripped sslmode when enforcement cannot be applied; keep
channel_binding removal unchanged.
In `@services/field-ops/src/field_ops/routers/logsheets.py`:
- Around line 234-242: Update the upload handling in the logsheets route so the
file size is validated before the current await file.read() call in the same
flow that saves via get_storage().save. Reject oversized uploads early using the
request’s file metadata or a streaming/chunked read path, and keep the existing
save order and storage_ref logic unchanged for valid uploads.
---
Outside diff comments:
In `@services/field-ops/frontend/src/components/LogSheetForm.tsx`:
- Around line 291-305: Guard the addToQueue call in the photo-upload failure
branch of onSubmit with QueueStorageError handling, matching the existing
offline and network-failure paths. On queue-storage failure, set submitState to
"saved" and surface the error through errorMsg so the form is no longer stuck in
"saving" and the photo remains available for retry.
---
Nitpick comments:
In `@services/field-ops/frontend/src/components/LogSheetForm.tsx`:
- Around line 109-117: Remove the empty inputStyle and readonlyStyle
declarations from LogSheetForm, then remove their corresponding style attributes
from the affected controls. Preserve the existing field.css selectors and all
form behavior so styling comes exclusively from the stylesheet.
In `@services/field-ops/src/field_ops/main.py`:
- Around line 32-36: Update the _ALLOWED_ORIGINS construction to include the
localhost origins only when settings.is_production is false; preserve the
configured non-wildcard CORS origins in all environments.
In `@services/field-ops/src/field_ops/storage.py`:
- Around line 80-86: Update the R2 client निर्माण in R2Storage’s boto3.client
call to pass an explicit botocore.config.Config instead of relying on defaults.
Set bounded connect/read timeouts and a small retry cap so uploads fail fast and
can be retried by the device queue, while keeping the existing endpoint_url,
credentials, and region_name behavior unchanged.
🪄 Autofix
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: 7e7a7442-1ee1-4744-a8ae-22a20477e0ce
⛔ Files ignored due to path filters (1)
services/field-ops/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.gitignorepyproject.tomlservices/field-ops/DEPLOY.mdservices/field-ops/frontend/index.htmlservices/field-ops/frontend/package.jsonservices/field-ops/frontend/src/App.tsxservices/field-ops/frontend/src/components/LogSheetForm.tsxservices/field-ops/frontend/src/components/LoginScreen.tsxservices/field-ops/frontend/src/components/QueueView.tsxservices/field-ops/frontend/src/hooks/useOfflineQueue.tsservices/field-ops/frontend/src/hooks/useTheme.tsservices/field-ops/frontend/src/main.tsxservices/field-ops/frontend/src/services/api.tsservices/field-ops/frontend/src/styles/field.cssservices/field-ops/frontend/vercel.jsonservices/field-ops/src/field_ops/config.pyservices/field-ops/src/field_ops/main.pyservices/field-ops/src/field_ops/routers/logsheets.pyservices/field-ops/src/field_ops/storage.py
| **Written 2026-08-06.** Target: a URL that field staff can open on their phones | ||
| before they leave, and that keeps working when they have no signal. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the architecture diagram fence.
Line 6 opens a fenced code block without a language. This triggers markdownlint MD040. Change the opener to ```text.
Proposed fix
-```
+```text🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 6-6: 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 `@services/field-ops/DEPLOY.md` at line 6, Update the opening fenced code block
in DEPLOY.md to specify the text language by changing the fence opener to
```text, resolving the MD040 markdownlint violation.
Source: Linters/SAST tools
| From this repo, with `DATABASE_URL` exported in your shell: | ||
|
|
||
| ```bash | ||
| export DATABASE_URL='postgresql://...' # from Neon, pooled |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline / relevant lines =="
wc -l services/field-ops/DEPLOY.md
sed -n '1,150p' services/field-ops/DEPLOY.md
echo
echo "== credential occurrences =="
rg -n "DATABASE_URL|R2_ACCESS_KEY_ID|R2_SECRET_ACCESS_KEY|fly secrets|secrets import|export " services/field-ops/DEPLOY.md || true
echo
echo "== surrounding scripts/config files =="
fd -a '(\.env|fly\.toml|fly\.toml\.sample|.*env.*|.*deploy.*|.*deploy)' services/field-ops || trueRepository: alfieprojectsdev/movefaults
Length of output: 6452
Do not guide operators to paste credentials into shell commands.
Line 56 exports DATABASE_URL in the shell, and lines 114-122 pass DATABASE_URL, R2_ACCESS_KEY_ID, and R2_SECRET_ACCESS_KEY directly to fly secrets set. These values can remain in shell history or process arguments. Use the provider’s secret-import/stdin flow or protected environment variables instead, such as Fly’s fly secrets import, which reads NAME=VALUE pairs from standard input.
🤖 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/field-ops/DEPLOY.md` at line 56, Update the deployment instructions
around the DATABASE_URL example and the fly secrets set usage to avoid putting
credentials in shell commands or arguments. Use Fly’s protected
secret-import/stdin flow, such as fly secrets import, with DATABASE_URL,
R2_ACCESS_KEY_ID, and R2_SECRET_ACCESS_KEY supplied through protected
environment variables or standard input.
| python3 -c "import bcrypt,secrets; pw=secrets.token_urlsafe(9); \ | ||
| print('password:', pw); print('hash:', bcrypt.hashpw(pw.encode(), bcrypt.gensalt()).decode())" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate DEPLOY.md and dependency/config files =="
fd -a 'DEPLOY\.md$|pyproject\.toml|uv\.lock|requirements.*|README.*' . | sed 's#^\./##' | head -200
echo
echo "== target lines =="
if [ -f services/field-ops/DEPLOY.md ]; then
nl -ba services/field-ops/DEPLOY.md | sed -n '150,180p'
fi
echo
echo "== relevant config files =="
for f in $(fd 'pyproject\.toml|uv\.lock|README.*' . | head -100); do
echo "--- $f"
sed -n '1,220p' "$f" | nl -ba | sed -n '1,220p'
done
echo
echo "== references to python3, uv run, bcrypt in deploy/docs =="
rg -n 'python3|uv run|uv|bcrypt|account|create|password|dependency' -S --glob '!node_modules' --glob '!dist' --glob '!build' .Repository: alfieprojectsdev/movefaults
Length of output: 1081
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target lines =="
awk 'NR>=150 && NR<=180 {printf "%5d\t%s\n", NR, $0}' services/field-ops/DEPLOY.md || sed -n '150,180p' services/field-ops/DEPLOY.md | awk '{printf "%5d\t%s\n", NR+149, $0}'
echo "== pyproject and lock files =="
for f in pyproject.toml uv.lock services/vadase-rt-monitor/uv.lock; do
[ -f "$f" ] || continue
echo "--- $f"
sed -n '1,260p' "$f"
done
echo "== project files =="
find . -maxdepth 3 \( -name 'pyproject.toml' -o -name 'uv.lock' \) -print | sort | while read -r f; do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo "== references =="
rg -n 'python3|uv run|uv|bcrypt|accounts|account|password|dependency|install|create' -S --glob '!node_modules' --glob '!dist' --glob '!build' .Repository: alfieprojectsdev/movefaults
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target lines =="
awk 'NR>=150 && NR<=180 {printf "%5d\t%s\n", NR, $0}' services/field-ops/DEPLOY.md
echo "== pyproject and uv.lock summaries (no rg across whole repo) =="
sed -n '1,220p' pyproject.toml
echo "--- uv.lock project packages around bcrypt/tools ---"
sed -n '1,260p' uv.lock | sed -n '/^name = /p;/^version = /p;/^source = /p;/^dependencies = /p;/^extra = /p;/optional = /' | head -200
echo "--- services/vadase-rt-monitor packages ---"
sed -n '1,220p' services/vadase-rt-monitor/pyproject.toml || true
sed -n '1,220p' services/vadase-rt-monitor/uv.lock || true
echo "== focused references in relevant docs and configs =="
rg -n 'python3|uv run|uv|bcrypt|accounts|account|password|dependency|install|create' -S pyproject.toml uv.lock services/field-ops/DEPLOY.md services/vadase-rt-monitor README.md || true
echo "== deterministic import-check behavior for clean host Python without bcrypt =="
python3 - <<'PY'
import importlib.util
spec = importlib.util.find_spec("bcrypt")
print("bcrypt_spec_found=", bool(spec))
PYRepository: alfieprojectsdev/movefaults
Length of output: 5771
Run password generation with the project interpreter.
This snippet imports bcrypt, but it does not use the project environment that declares field-ops deps. A clean host with only python3 can fail this step. Run it with uv run python -c ..., or add an explicit dependency check before the command.
Proposed command change
-python3 -c "import bcrypt,secrets; pw=secrets.token_urlsafe(9); \
+uv run python -c "import bcrypt,secrets; pw=secrets.token_urlsafe(9); \📝 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.
| python3 -c "import bcrypt,secrets; pw=secrets.token_urlsafe(9); \ | |
| print('password:', pw); print('hash:', bcrypt.hashpw(pw.encode(), bcrypt.gensalt()).decode())" | |
| uv run python -c "import bcrypt,secrets; pw=secrets.token_urlsafe(9); \ | |
| print('password:', pw); print('hash:', bcrypt.hashpw(pw.encode(), bcrypt.gensalt()).decode())" |
🤖 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/field-ops/DEPLOY.md` around lines 169 - 170, Update the
password-generation command in the deployment instructions to invoke the project
interpreter via uv run python, ensuring the declared field-ops dependencies such
as bcrypt are available.
Source: Coding guidelines
| - **No frontend tests.** The slant→RH computation is the only real domain | ||
| arithmetic in the UI and has none. A wrong answer there silently corrupts the | ||
| vertical component of every campaign occupation. It is verified by hand for | ||
| `TRM55971-00` only. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'slant|TRM55971|relative.?humidity|\bRH\b' services/field-ops/frontend/src || true
rg -n -C 3 'vitest|jest|playwright|test:' \
services/field-ops/frontend/package.json services/field-ops/frontend || trueRepository: alfieprojectsdev/movefaults
Length of output: 17354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== LogSheetForm slice =="
sed -n '1,80p' services/field-ops/frontend/src/components/LogSheetForm.tsx
sed -n '160,235p' services/field-ops/frontend/src/components/LogSheetForm.tsx
echo
echo "== frontend package and test files =="
fd -a '^(package\.json|vitest\.config\..*|jest\.config\..*|playwright\.config\..*|setup.*\.(js|ts|mjs|cjs))$' services/field-ops/frontend | sed 's#^\./##' | sort
sed -n '1,180p' services/field-ops/frontend/package.json 2>/dev/null || true
rg -n -C 2 'describe|it\s*\(|test\s*\(|expect\(|const .*to\s*be|assert' services/field-ops/frontend/src services/field-ops/frontend 2>/dev/null || true
echo
echo "== deploy lines around test coverage note =="
sed -n '200,235p' services/field-ops/DEPLOY.md
echo
echo "== static search for calculateHelper / antenna math in tests and src =="
rg -n 'calculate|antenna|rinex_height|avg_slant|slantN|VO|Math\.sqrt|rhValue' services/field-ops/frontend/src services/field-ops/frontend -g '!node_modules' || trueRepository: alfieprojectsdev/movefaults
Length of output: 21260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== submit validation slice =="
sed -n '235,290p' services/field-ops/frontend/src/components/LogSheetForm.tsx
echo
echo "== LogSheetIn schema in api =="
sed -n '35,70p' services/field-ops/frontend/src/services/api.ts
echo
echo "== JS numeric/NaN behavior used by current slant parser =="
node - <<'JS'
const samples = [
["0", "0", "0", "0"],
["0", "0", "0", "0.1698"],
["0", "0", "0", "0.1699"],
["1", "1", "1", "1"],
["0", "", "0", "0"],
["0", "+.", "0", "0"],
["0", "-", "0", "0"],
["0", "1.4320", "1.4318", "1.4322"],
];
for (const [N,E,S,W] of samples) {
const Nf = parseFloat(N), Ef = parseFloat(E), Sf = parseFloat(S), Wf = parseFloat(W);
const allSlantsFilled = Nf === Nf && Ef === Ef && Sf === Sf && Wf === Wf;
const avgSH = allSlantsFilled ? (Nf + Ef + Sf + Wf) / 4 : undefined;
const C = 0.1698, VO = 0.0444;
const rh = avgSH !== undefined ? Math.sqrt(avgSH * avgSH - C * C) - VO : undefined;
console.log(JSON.stringify({N,E,S,W,Nf,Ef,Sf,Wf,allSlantsFilled,avgSH,rh}));
}
JSRepository: alfieprojectsdev/movefaults
Length of output: 4447
Add automated coverage for the slant-to-RH calculation before field deployment.
The campaign submit path computes and submits avg_slant_m and rinex_height_m directly with no frontend tests or test runner in the frontend package. Add TDD-style coverage for each supported antenna model, normal and boundary slant values, and invalid or partial inputs before release.
🤖 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/field-ops/DEPLOY.md` around lines 220 - 223, Add frontend
test-runner coverage for the campaign submit calculation that produces
avg_slant_m and rinex_height_m. Add TDD-style tests covering every supported
antenna model, normal and boundary slant values, plus invalid and partial
inputs, and ensure the tests assert the submitted values. Configure the frontend
package to run these tests before field deployment.
Source: Coding guidelines
| - **Photos are never deleted.** No retention policy; at ~3 MB each the 10 GB | ||
| free tier is fine for this trip and will need a decision later. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Resolve photo retention and archive protection before deployment.
Lines 231-232 state that photos are never deleted. Lines 240-243 export the database and photos to local paths without an explicit encryption, access-control, or cleanup step. Lines 89-90 state that photos can show people. Define a retention period and deletion procedure for both R2 objects and field_ops.logsheet_photos. Specify encrypted, access-controlled archive handling before field use.
Also applies to: 240-243
🤖 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/field-ops/DEPLOY.md` around lines 231 - 232, Update the deployment
documentation covering photo storage, database/photo exports, and the
people-visible photo handling to define a retention period and deletion
procedure for both R2 objects and field_ops.logsheet_photos. Specify that
exported archives are encrypted, access-controlled, and securely cleaned up
after use before field deployment, replacing the current “never deleted”
guidance.
| "source": "/api/:path*", | ||
| "destination": "https://REPLACE-WITH-BACKEND-HOST/api/:path*" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if rg -n 'REPLACE-WITH-BACKEND-HOST' services/field-ops/frontend/vercel.json; then
echo "The Vercel API target is still a placeholder." >&2
exit 1
fiRepository: alfieprojectsdev/movefaults
Length of output: 281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== vercel.json =="
cat -n services/field-ops/frontend/vercel.json
echo
echo "== DEPLOY references =="
rg -n -C 3 'REPLACE-WITH-BACKEND-HOST|backend host|backend-host|BACKEND|vercel.json|deploy' services/field-ops services 2>/dev/null | head -n 240
echo
echo "== deployment automation references to field-ops vercel/json =="
rg -n 'services/field-ops/frontend/vercel.json|field-ops|REPLACE-WITH-BACKEND-HOST|vercel.json' -g '!node_modules' -g '!dist' -g '!build' | head -n 240Repository: alfieprojectsdev/movefaults
Length of output: 19083
Add a guard for the Vercel backend placeholder.
services/field-ops/frontend/vercel.json still rewrites /api/* to https://REPLACE-WITH-BACKEND-HOST/api/:path*. The manual Vercel deployment step replaces this value, but the repository does not reject it, so a missed edit breaks every backend request. Add template substitution or a CI/pre-deploy check that fails before release if vercel.json still contains REPLACE-WITH-BACKEND-HOST.
services/field-ops/frontend/vercel.json:9
🤖 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/field-ops/frontend/vercel.json` around lines 8 - 9, Add a CI or
pre-deploy validation for the frontend Vercel configuration that searches the
API rewrite destination in vercel.json for REPLACE-WITH-BACKEND-HOST and fails
before release when the placeholder remains; preserve the existing manual
substitution flow and valid backend rewrite behavior.
| # asyncpg rejects libpq's ?sslmode= parameter; it negotiates TLS via | ||
| # its own `ssl` argument. Neon's copy-paste string includes it, and | ||
| # leaving it in produces a confusing connect error at first request. | ||
| if "?" in url: | ||
| base, _, query = url.partition("?") | ||
| kept = [ | ||
| kv for kv in query.split("&") | ||
| if kv and not kv.startswith(("sslmode=", "channel_binding=")) | ||
| ] | ||
| url = base + ("?" + "&".join(kept) if kept else "") | ||
| return url |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
asyncpg default ssl mode prefer sslmode SQLAlchemy asyncpg connect_args ssl
💡 Result:
In asyncpg, the default SSL mode is 'prefer' [1][2], which means the driver will attempt an SSL connection first and fall back to a non-SSL connection if the SSL handshake fails [1][2]. This behavior was introduced in asyncpg version 0.22.0 to align with libpq standards [1][3]. When using asyncpg with SQLAlchemy via create_async_engine, it is important to note that the 'sslmode' parameter—commonly used in DSN strings for other drivers like psycopg2—is not natively supported as a keyword argument in the asyncpg connect method [4][5]. Instead, asyncpg's connect method uses an 'ssl' keyword argument [1][4]. To configure SSL settings when using SQLAlchemy and asyncpg, use the connect_args dictionary parameter in create_async_engine [6][7]. 1. Simple SSL configuration: If you want to ensure SSL is used (equivalent to requiring SSL), pass ssl=True within connect_args [1][5]: engine = create_async_engine(DATABASE_URL, connect_args={"ssl": True}) 2. Advanced SSL configuration: For complex requirements such as custom CA certificates or client-side certificates, you must create an ssl.SSLContext object and pass it to the 'ssl' key in connect_args [6][8]: import ssl ssl_ctx = ssl.create_default_context(cafile="path/to/ca.pem") engine = create_async_engine(DATABASE_URL, connect_args={"ssl": ssl_ctx}) Note that while asyncpg internally recognizes 'sslmode' when parsing a DSN string, SQLAlchemy's URL parsing does not always pass 'sslmode' query parameters correctly to the underlying asyncpg driver [4][8][5]. Therefore, configuring SSL explicitly through connect_args is the recommended approach for reliability [8][5].
Citations:
- 1: https://magicstack.github.io/asyncpg/current/api/
- 2: https://magicstack.github.io/asyncpg/current/api/index.html
- 3: Prefer SSL connections by default MagicStack/asyncpg#660
- 4: sslmode query param does not work for asyncpg DBAPI sqlalchemy/sqlalchemy#6275
- 5: asyncpg does not work with "sslmode" query param when called from SQLAlchemy MagicStack/asyncpg#737
- 6: Cannot pass SSL context to asyncpg from
create_async_enginein SQLAlchemy 1.4 sqlalchemy/sqlalchemy#5973 - 7: http://docs.sqlalchemy.org/en/latest/core/engines.html
- 8: using client-side certificates with postgresql+asyncpg sqlalchemy/sqlalchemy#10894
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline services/field-ops/src/field_ops/config.py --view expanded || true
echo "== relevant config.py sections =="
sed -n '1,140p' services/field-ops/src/field_ops/config.py | cat -n
echo "== database_url occurrences =="
rg -n "database_url|create_async_engine|asyncpg|sslmode|channel_binding|connect_args" services/field-ops/src/field_ops -SRepository: alfieprojectsdev/movefaults
Length of output: 9016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant database.py =="
sed -n '1,120p' services/field-ops/src/field_ops/database.py | cat -n
echo "== repository dependency/version hints =="
for f in pyproject.toml uv.lock requirements.txt services/field-ops/pyproject.toml services/field-ops/uv.lock services/field-ops/requirements.txt; do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,220p' "$f" | rg -n "sqlalchemy|asyncpg|pydantic|uv" -C 2 || true; }
doneRepository: alfieprojectsdev/movefaults
Length of output: 10843
Preserve the TLS requirement when normalizing database_url.
database_url is produced for create_async_engine(...) without any connect_args, so dropping sslmode=require changes sslmode=nil for asyncpg’s default prefer behavior. That can fall back to plain-text connections and does not enforce the provider’s TLS requirement.
Keep the TLS intent: strip sslmode= from the URL, but record the original value and apply an equivalent asyncpg TLS setting, such as connect_args={"ssl": ssl.create_default_context()} for required/verify-ca modes or a certificate check for peer verification. Store the stripped value in logs when TLS is required but unenforceable from the URL.
🤖 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/field-ops/src/field_ops/config.py` around lines 69 - 79, Update the
database URL normalization flow to preserve TLS semantics when removing sslmode
parameters: capture the original sslmode value, strip it from the URL, and
propagate required or certificate-verifying modes through the
create_async_engine connection configuration using an appropriate asyncpg SSL
context. Ensure TLS-required configurations are not downgraded to plaintext, and
log the stripped sslmode when enforcement cannot be applied; keep
channel_binding removal unchanged.
| @property | ||
| def is_production(self) -> bool: | ||
| return self.field_ops_production == "1" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
is_production fails open on any value other than "1".
FIELD_OPS_PRODUCTION=true or FIELD_OPS_PRODUCTION=1 (trailing space) makes is_production false. All checks in _assert_deployable are then skipped, and the instance boots with the published default JWT secret on a public URL. A gate that a value typo disables is not a gate.
Accept a set of truthy values and normalize the input.
🔒 Proposed fix
`@property`
def is_production(self) -> bool:
- return self.field_ops_production == "1"
+ # Any conventional truthy spelling enables the gate. Only an explicitly
+ # absent or falsey value disables it, so a typo cannot open the service.
+ return self.field_ops_production.strip().lower() in {"1", "true", "yes", "on"}📝 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.
| @property | |
| def is_production(self) -> bool: | |
| return self.field_ops_production == "1" | |
| `@property` | |
| def is_production(self) -> bool: | |
| # Any conventional truthy spelling enables the gate. Only an explicitly | |
| # absent or falsey value disables it, so a typo cannot open the service. | |
| return self.field_ops_production.strip().lower() in {"1", "true", "yes", "on"} |
🤖 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/field-ops/src/field_ops/config.py` around lines 86 - 88, Update the
is_production property to normalize field_ops_production by trimming whitespace
and applying case normalization, then treat the accepted truthy values
(including "1" and "true") as production. Ensure invalid or unrecognized values
do not disable production checks in _assert_deployable.
| if s.field_ops_storage_backend.lower() != "r2": | ||
| problems.append( | ||
| "FIELD_OPS_STORAGE_BACKEND must be 'r2' in production — a container " | ||
| "filesystem is ephemeral, so photos written to disk are lost on restart" | ||
| ) | ||
|
|
||
| if not s.database_url: | ||
| problems.append("DATABASE_URL is unset") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The production gate does not validate the R2 credentials it requires.
The gate requires field_ops_storage_backend == "r2" but never checks r2_account_id, r2_access_key_id, r2_secret_access_key, or r2_bucket. get_storage() in services/field-ops/src/field_ops/storage.py performs that check, and it is first called inside upload_photo. A production instance with an incomplete R2 configuration therefore boots, accepts logsheets, and fails only when an operator uploads the first photo. That contradicts the fail-closed claim in the storage.py module docstring (lines 22-26).
Add the credential names to the production problem list, or resolve the backend once during startup so the existing RuntimeError in get_storage() fires before the service serves traffic.
🛡️ Proposed fix
if s.field_ops_storage_backend.lower() != "r2":
problems.append(
"FIELD_OPS_STORAGE_BACKEND must be 'r2' in production — a container "
"filesystem is ephemeral, so photos written to disk are lost on restart"
)
+ else:
+ for name, value in (
+ ("R2_ACCOUNT_ID", s.r2_account_id),
+ ("R2_ACCESS_KEY_ID", s.r2_access_key_id),
+ ("R2_SECRET_ACCESS_KEY", s.r2_secret_access_key),
+ ("R2_BUCKET", s.r2_bucket),
+ ):
+ if not value:
+ problems.append(f"{name} is unset (required by FIELD_OPS_STORAGE_BACKEND=r2)")📝 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.
| if s.field_ops_storage_backend.lower() != "r2": | |
| problems.append( | |
| "FIELD_OPS_STORAGE_BACKEND must be 'r2' in production — a container " | |
| "filesystem is ephemeral, so photos written to disk are lost on restart" | |
| ) | |
| if not s.database_url: | |
| problems.append("DATABASE_URL is unset") | |
| if s.field_ops_storage_backend.lower() != "r2": | |
| problems.append( | |
| "FIELD_OPS_STORAGE_BACKEND must be 'r2' in production — a container " | |
| "filesystem is ephemeral, so photos written to disk are lost on restart" | |
| ) | |
| else: | |
| for name, value in ( | |
| ("R2_ACCOUNT_ID", s.r2_account_id), | |
| ("R2_ACCESS_KEY_ID", s.r2_access_key_id), | |
| ("R2_SECRET_ACCESS_KEY", s.r2_secret_access_key), | |
| ("R2_BUCKET", s.r2_bucket), | |
| ): | |
| if not value: | |
| problems.append(f"{name} is unset (required by FIELD_OPS_STORAGE_BACKEND=r2)") | |
| if not s.database_url: | |
| problems.append("DATABASE_URL is unset") |
🤖 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/field-ops/src/field_ops/config.py` around lines 115 - 122, Update
the production validation in the settings/configuration flow to require
non-empty r2_account_id, r2_access_key_id, r2_secret_access_key, and r2_bucket
whenever field_ops_storage_backend is "r2"; append each missing credential to
the existing problems list so startup fails before serving traffic, while
preserving the current backend and DATABASE_URL checks.
| contents = await file.read() | ||
| dest.write_bytes(contents) | ||
|
|
||
| # Store the bytes BEFORE the row. If this raises, the request fails and the | ||
| # device keeps the photo queued for retry. The reverse order would commit a | ||
| # row referencing an object that was never written — the DB would look | ||
| # correct and the photo would be gone. | ||
| storage_ref = await get_storage().save( | ||
| logsheet_id, file.filename or "photo.jpg", contents | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the upload size before reading the file into memory.
await file.read() loads the whole upload into process memory. Phone photos are several megabytes, and there is no size or content-type limit on this route. Several concurrent uploads on a small container can exhaust memory and restart the instance, which is the worst moment for a field device that is mid-sync.
Reject oversized uploads early, and stream or chunk the read.
🛡️ Proposed fix
- contents = await file.read()
+ # Bounded read: an unbounded photo would sit entirely in process memory.
+ max_bytes = 15 * 1024 * 1024
+ contents = await file.read(max_bytes + 1)
+ if len(contents) > max_bytes:
+ raise HTTPException(status_code=413, detail="Photo exceeds 15 MB limit")📝 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.
| contents = await file.read() | |
| dest.write_bytes(contents) | |
| # Store the bytes BEFORE the row. If this raises, the request fails and the | |
| # device keeps the photo queued for retry. The reverse order would commit a | |
| # row referencing an object that was never written — the DB would look | |
| # correct and the photo would be gone. | |
| storage_ref = await get_storage().save( | |
| logsheet_id, file.filename or "photo.jpg", contents | |
| ) | |
| # Bounded read: an unbounded photo would sit entirely in process memory. | |
| max_bytes = 15 * 1024 * 1024 | |
| contents = await file.read(max_bytes + 1) | |
| if len(contents) > max_bytes: | |
| raise HTTPException(status_code=413, detail="Photo exceeds 15 MB limit") | |
| # Store the bytes BEFORE the row. If this raises, the request fails and the | |
| # device keeps the photo queued for retry. The reverse order would commit a | |
| # row referencing an object that was never written — the DB would look | |
| # correct and the photo would be gone. | |
| storage_ref = await get_storage().save( | |
| logsheet_id, file.filename or "photo.jpg", contents | |
| ) |
🤖 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/field-ops/src/field_ops/routers/logsheets.py` around lines 234 -
242, Update the upload handling in the logsheets route so the file size is
validated before the current await file.read() call in the same flow that saves
via get_storage().save. Reject oversized uploads early using the request’s file
metadata or a streaming/chunked read path, and keep the existing save order and
storage_ref logic unchanged for valid uploads.
1 participant
Prepares the logsheet PWA for a temporary deployment supporting fieldwork in
Palawan next week, and fixes three defects found while auditing it against
real field use rather than against a desk with signal.
Three commits, reviewable independently.
fix(field-ops)— data loss on the offline pathThis is the one worth reviewing carefully. All three defects would have bitten
in Palawan, where offline is the normal case rather than the exception.
1. Offline submissions destroyed the mandatory photo
The offline path queued
record— which isLogSheetIn, and has no photofield — then called
reset(), clearing the file input. The blob was gone fromthe browser entirely, while the UI reported:
True of the text. False of the photo. And there was no recovery: the Queue view
was a
TODOstub, so nothing listed what was pending or allowed a re-attach.A photo is required to submit. So the app enforced a photo, then silently
discarded it on the exact path field staff use most.
The blob now lives in IndexedDB with the record (schema v1 → v2, an additive
upgrade that carries existing pending records forward — recreating the store
would discard unsynced fieldwork) and uploads after the logsheet POST.
Exactly-once under retry. The logsheet POST is idempotent server-side
(
ON CONFLICT (client_uuid) DO NOTHING, then re-fetch); photo upload has no suchguard.
_photoUploadedis persisted before the record is marked synced, so acrash between the two leaves it pending with the photo flagged done — the next
flush re-POSTs the logsheet harmlessly and skips the photo. Verified against the
running API: same
client_uuidtwice → same id, one row.A record is marked synced only once both halves are on the server.
2. There was no way to log in
login()has existed inapi.tssince the service was scaffolded. Nothingcalled it; no form existed. The only way to use the app was to mint a JWT by
hand into
localStorage— not something a field observer can do on a phone at amonument.
3. Nothing showed what was queued
The Queue tab now lists each record with photo state and age, shows device
storage headroom, and offers manual sync — signal often returns as a brief
window and the browser's own
onlineevent is not always fast enough to catchit.
feat(field-ops)— R2 storage and a fail-closed production gatePhotos no longer go to local disk.
write_bytes()under/tmpis right on aworkstation and wrong on every deployment target: a container filesystem is
ephemeral, so
logsheet_photosrows would point at files the next restart hadalready destroyed. The DB would look healthy and the evidence would be gone.
Bytes are written before the row, so a failed upload fails the request rather
than committing a reference to an object that was never stored.
The startup gate is the important part.
FIELD_OPS_PRODUCTION=1refuses toboot if the JWT secret is the shipped default or under 32 chars, if storage is
not R2, or if
DATABASE_URLis unset.This repo is public. With the default secret, anyone who reads it can mint a
valid token for the deployed URL and post logsheets as any user. A service that
boots happily in that state is worse than one that will not boot, because nobody
finds out. Verified by execution — all conditions fire together, correct config
boots, dev mode unaffected, and the message names variables without ever printing
a value.
Three things that would have broken the deploy on arrival:
port=8001hardcoded(container hosts assign
PORTand route to it — health check never passes,deploy rolls back),
reload=Trueunconditional (restarts the processmid-request), and CORS hardcoded to localhost.
DEPLOY.mdis the runbook. Every step needing an account or credential is marked[you]; only secret names appear in this repo. §6 is a phone-based checklist
whose airplane-mode step is the one that matters — it exercises the path fixed
above, whose failure mode is silent.
feat(field-ops)— styling (unchanged from earlier review)Pico.css + a field-use layer + three-state theme. 12.8 kB gzipped.
Verification
verified by running them, not by reading
unauthenticated 401
Not covered: there are still no frontend tests. The slant→RH computation is
the only real domain arithmetic in the UI and has none — a wrong answer there
silently corrupts the vertical component of every campaign occupation. Verified
by hand for
TRM55971-00only.Note for reviewers — line endings
Staging showed
LogSheetForm.tsxat 1,358 changed lines andlogsheets.pyat501. Both were CRLF→LF normalisation by my tooling; restored, the real diffs
are 52 and 29.
This is the second time in two sessions. There is no
.gitattributes, thefrontend is CRLF, and roughly half the sampled
.pyfiles are too. With twomachines committing — and gps3 being Linux — this will keep producing
unreviewable whole-file diffs. Worth its own change; deliberately not folded in
here, since normalising touches many files and deserves to be reviewed alone.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation