feat: field amendments MVP — extraction, pricing, signing, PDF to Drive - #1
Conversation
Settings is the single environment surface all later skipta modules consume, centralizing configuration from env vars and optional .env files, with sensible defaults for model names, output tokens, and rate limits. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Standalone probe for the service-account plumbing: run with impersonated ADC after sharing the pricing spreadsheet with skipta-gsa@ — printing the tab list proves scopes, sharing, and impersonation end to end before any app code touches Google. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Structured output pins the schema at the API layer, the model fallback chain absorbs quota/availability failures, and hallucination control is deferred to deterministic pricing. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Extraction identifies spec names only; pricing establishes what exists in inventory and its cost. A spec that fails to match a pricing row produces a visible UNMATCHED line item with zero cost, blocking downstream signing until the row is added to the Panels or Breakers tab. This guard prevents enacting amendment clauses for electrically-exotic or LLM-invented parts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Google clients are centralized in one module so all downstream services take injected clients, enabling seamless testing with fakes. The Amendments tab carries all durable state—one row per amendment with lifecycle from draft to signed—keeping each pod stateless and allowing horizontal scaling without coordination. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Task 7 review caught the apostrophe character class holding U+0027 twice instead of straight + curly (U+2019), so phone-keyboard curly apostrophes became hyphens in slugs. Pattern corrected and the slug test now covers the curly case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ensure_customer_folder keeps the customer-folder contract instead of silently filing at root. find_file_in_folder lets the sign flow reuse an already-uploaded PDF so retries never duplicate. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The same Jinja template renders both the on-screen signing preview and the flattened PDF via `render_amendment_html` / `render_pdf`; WeasyPrint stays behind a lazy import inside `render_pdf` so GTK-less hosts (this one included) can still exercise every other code path. Tests are split into two files rather than the single-file guarded layout the brief sketches, because on this host `import weasyprint` itself raises `OSError` (missing `libgobject-2.0-0`) rather than `ImportError` — a module-level `pytest.mark.skip` in a combined file would skip the whole file, including the HTML-contents test that has to serve as the local RED/GREEN vehicle. `tests/test_pdf_html.py` never imports weasyprint and always runs; `tests/test_pdf.py` guards the import and skips the PDF-bytes assertion locally while running for real in CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Assets are vendored (signature_pad bundled locally rather than pulled from a CDN) so a job site with weak signal never blocks on a network fetch mid-signature. The signing page disables enactment whenever the amendment has unmatched pricing items or has already been signed, so a crew member can't sign off on incomplete data or double-sign. Templates are exercised by Task 11's route tests, not here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Routes stay thin over the sibling modules — extraction, pricing, and amendments do the work; `app/main.py` only wires dependency providers and shapes HTTP in/out. The dependency providers (`get_settings`, `get_sheets`, `get_drive`, `get_extract`) exist so tests inject fakes via `app.dependency_overrides` without patching. The customer-name contract (blank/whitespace → None → extraction fallback → 422) prevents unnamed `amend__…` ids. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…warning Task 11 review surfaced that the limiter's limit-lambda calls get_settings() directly, outside Depends resolution, so dependency_overrides never reaches it — the whole suite shared one real 10/min budget and Task 12's added POSTs would have started drawing 429s. Rate limiting is prod behavior, so the client fixture disables the limiter and restores it on teardown. Also filters the Starlette TestClient deprecation warning (third-party, unactionable) to keep test output pristine. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dual-signature POST closes the loop: render with embedded signatures, flatten via WeasyPrint, deterministic filename + pre-upload lookup keep retries idempotent, find-or-create keeps the customer-folder contract, Sheets row flips draft→signed with the Drive link. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Slim python:3.11 base with the pango/cairo runtime libs WeasyPrint needs for PDF rendering, plus a real font so output isn't boxes. CI runs ruff and the full pytest suite — including the real PDF render — on an ubuntu runner with the same GTK libs installed. The image workflow follows the ting ghcr pattern: build/push on main pushes, tagged by sha and latest, with GitHub Actions layer caching. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds a FastAPI amendment workflow backed by Google Sheets, Google Drive, Vertex AI extraction, HTML/PDF rendering, web templates, tests, and container/Kubernetes deployment assets. ChangesSkipta Application
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Skipta API
participant Vertex as extract_amendment
participant Sheets as Google Sheets
participant Pricing as price_amendment
Client->>API: POST /api/v1/amendments
API->>Vertex: extract_amendment(voice_text)
Vertex-->>API: AmendmentPayload
API->>Sheets: read_values(Panels, Breakers)
Sheets-->>API: pricing rows
API->>Pricing: price_amendment(payload, panels, breakers)
Pricing-->>API: PricingResult
API->>Sheets: append_amendment(draft record)
API-->>Client: 201 {amendment_id, signing_url}
sequenceDiagram
participant Client
participant API as Skipta API
participant Sheets as Google Sheets
participant PDF as render_pdf
participant Drive as Google Drive
Client->>API: POST /api/v1/amendments/{id}/sign
API->>Sheets: find_amendment(id)
Sheets-->>API: amendment record
API->>PDF: render_amendment_html + render_pdf
PDF-->>API: pdf_bytes
API->>Drive: ensure_customer_folder(customer_name)
Drive-->>API: folder_id
API->>Drive: find_file_in_folder / upload_pdf
Drive-->>API: webViewLink
API->>Sheets: mark_signed(row, pdf_url, signed_at)
API-->>Client: 200 {pdf_drive_url}
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…cmdbee.org ting-shaped kustomize base: Workload Identity KSA (skipta-sa → skipta-gsa), single-replica Deployment with healthz probes and WeasyPrint-sized resources, ClusterIP service, and an HTTPRoute on the shared traefik-gateway web+websecure listeners — the platform wildcard *.cmdbee.org cert covers TLS, so no per-host Certificate or ReferenceGrant. ConfigMap carries identifiers only (spreadsheet + Drive folder IDs, project/region, base URL) — no secrets anywhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements the Skipta “field amendments” MVP end-to-end: intake → Gemini extraction → deterministic Sheets-based pricing (with UNMATCHED guard) → signing UI → PDF render → upload to customer Drive folder, plus CI/image build and k8s base manifests.
Changes:
- Adds FastAPI routes + supporting modules for extraction, pricing, signing flow, PDF rendering, and Google Drive/Sheets integration.
- Adds web UI (index + signing pages), styling, and a vendored signature pad script.
- Adds comprehensive pytest coverage, container build, GitHub Actions CI/image workflows, and kustomize base manifests.
Reviewed changes
Copilot reviewed 35 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_routes_sign.py | Tests signing endpoint behavior (Drive upload, 409 double-sign, retry semantics). |
| tests/test_routes_intake.py | Tests health/index, amendment creation, signing page rendering, and input validation. |
| tests/test_pricing.py | Tests deterministic pricing matching and UNMATCHED behavior. |
| tests/test_pdf.py | Tests PDF rendering output and handles missing WeasyPrint system deps. |
| tests/test_pdf_html.py | Tests HTML template rendering content for PDFs. |
| tests/test_extraction.py | Tests extraction fallback chain and schema validation error behavior. |
| tests/test_drive.py | Tests Drive folder/file lookup and upload helper behavior. |
| tests/test_config.py | Tests env-driven Settings parsing (including model list splitting). |
| tests/test_amendments.py | Tests Amendments sheet row model, ID generation, append/find, and mark_signed update. |
| tests/conftest.py | Provides fakes and dependency overrides for integration-style route tests. |
| tests/init.py | Marks tests package. |
| scripts/verify_access.py | Adds a one-shot script to verify Sheets access via ADC credentials. |
| pyproject.toml | Adds pytest warning filter configuration. |
| k8s/base/serviceaccount.yaml | Adds ServiceAccount with GKE Workload Identity annotation. |
| k8s/base/service.yaml | Adds ClusterIP service for the app. |
| k8s/base/namespace.yaml | Adds the skipta namespace manifest. |
| k8s/base/kustomization.yaml | Adds kustomize base wiring for namespace/app resources. |
| k8s/base/httproute.yaml | Adds Gateway API HTTPRoute for skipta.cmdbee.org. |
| k8s/base/deployment.yaml | Adds deployment manifest (latest image, env from ConfigMap, probes, resources). |
| k8s/base/configmap.yaml | Adds ConfigMap containing GCP/Sheet/Drive/base_url values. |
| Dockerfile | Adds slim runtime image setup including WeasyPrint runtime libs. |
| app/templates/sign.html | Adds signing page UI (line items + signature capture + sign POST). |
| app/templates/index.html | Adds intake form UI to create an amendment. |
| app/templates/amendment_pdf.html | Adds PDF HTML template (line items + signatures). |
| app/static/skipta.css | Adds minimal styling for intake/signing pages. |
| app/static/signature_pad.umd.min.js | Vendors signature_pad for local/offline signing UI. |
| app/pricing.py | Implements deterministic pricing and UNMATCHED guard logic. |
| app/pdf.py | Implements template-based HTML rendering and WeasyPrint PDF generation. |
| app/main.py | Adds FastAPI routes, dependency injection, rate limiting, signing flow orchestration. |
| app/google_clients.py | Centralizes Google client construction and Sheets value reads. |
| app/extraction.py | Implements Gemini structured extraction with model fallback chain. |
| app/drive.py | Implements Drive folder lookup/create and idempotent PDF upload/lookup. |
| app/config.py | Adds env-driven Settings dataclass with defaults and parsing. |
| app/amendments.py | Implements Amendments tab row schema + CRUD-ish helpers and state update. |
| app/init.py | Marks app package. |
| .github/workflows/image.yml | Adds GHCR image build/push workflow. |
| .github/workflows/ci.yml | Adds CI workflow (ruff + pytest + WeasyPrint system deps). |
| .dockerignore | Adds docker ignore rules for local/dev/test artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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 @.github/workflows/ci.yml:
- Around line 1-17: Harden the ci workflow by tightening GitHub Actions security
settings in the existing jobs setup: add an explicit permissions block with the
minimum required scopes, pin the actions used by actions/checkout and
actions/setup-python to immutable commit SHAs instead of version tags, set
persist-credentials to false on the checkout step, and add a concurrency group
for the workflow so older runs are canceled when newer commits arrive.
In @.github/workflows/image.yml:
- Around line 5-7: The workflow grants packages: write too broadly and uses
unpinned third-party actions, so tighten the token scope and secure the action
refs. In the image workflow, keep packages: write only where it is needed for
docker/build-push-action, add a clear comment justifying that permission, and
replace the `@v4/`@v3/@v6 references in the workflow steps with pinned commit SHAs
for each action such as checkout, setup-buildx, login, and build-push.
In `@app/config.py`:
- Around line 23-34: The Settings.from_env() loader currently hides missing
required values by defaulting project_id, spreadsheet_id, and drive_folder_id to
empty strings. Update from_env() to validate these required env vars explicitly
and raise a clear startup error if any are missing, while keeping the existing
parsing for optional fields like region, base_url, model_names,
max_output_tokens, and rate_limit_per_minute.
In `@app/drive.py`:
- Around line 13-20: find_customer_folder is using a prefix-style Drive query
via name contains, which can match the wrong folder; change the query in
find_customer_folder to use exact name matching instead so it only returns
folders whose name exactly equals customer_name. Keep the existing
root_folder_id, FOLDER_MIME, and trashed filters intact, and preserve the
current return behavior from files[0]["id"] when a match is found.
In `@app/extraction.py`:
- Around line 64-65: The import of vertexai.generative_models in
extract_amendment and the related setup in google_clients are relying on a
module that is not guaranteed in newer google-cloud-aiplatform 1.x releases.
Update these call sites to use the Google Gen AI SDK instead, or pin
google-cloud-aiplatform to a version that still provides
vertexai.generative_models so imports do not fail on fresh installs. Use the
extract_amendment function and the google_clients module as the primary places
to change.
- Around line 64-80: Add a per-call timeout to the extraction flow so a hung
model call cannot block the fallback loop in extract_amendment(). Update
extract_amendment() to enforce a deadline around each
model_factory(name).generate_content(...) invocation, or use a client/API that
supports timeouts directly, and keep the sequential retry behavior intact. Make
sure the timeout is applied per model attempt so create_amendment() returns or
falls back promptly instead of waiting indefinitely.
In `@app/main.py`:
- Around line 169-190: The sign flow in the amendment handler has a
check-then-act race: `find_amendment`, the `record.status == "signed"` guard,
and the later `mark_signed` call can be hit concurrently, allowing duplicate PDF
uploads for the same `amendment_id`. Fix this by adding an atomic re-check or
optimistic-lock style precondition immediately before `amendments.mark_signed`
in the same handler, and if the row is already signed, skip the Drive upload
path and return the conflict response. Also make sure the UI-side double-click
protection in `sign.html` is paired with this server-side safeguard.
- Around line 117-119: The amendment pricing flow in create-amendment currently
calls read_values for the Panels and Breakers sheets on every request, causing
repeated full-sheet reads and quota pressure. Add a short-TTL in-memory cache
around the pricing tab fetches in the amendment handler path (where panels,
breakers, and price_amendment are used), so repeated POST /api/v1/amendments
requests reuse recent data. Keep the cache keyed by spreadsheet_id and sheet
range, and provide a clear invalidation path (timer-based expiry or a manual
refresh action) so pricing stays reasonably fresh.
- Around line 36-57: The shared caches in get_settings, get_sheets, and
get_drive are vulnerable to check-then-act races under concurrent Starlette
threadpool execution. Replace the manual _settings/_clients dict caching with a
thread-safe one-shot cache such as functools.lru_cache(maxsize=1), or otherwise
synchronize access so each of Settings.from_env, build_sheets, and build_drive
is initialized only once. Keep the existing function names so callers continue
using the same entry points.
In `@app/pricing.py`:
- Around line 28-41: The row parsers in parse_panels and parse_breakers
currently assume every numeric cell is clean, so a single malformed sheet row
can crash the entire pricing flow. Update these helpers to validate and safely
parse the numeric fields (panel max_amperage/unit_cost, breaker
amps/poles/unit_cost) and skip bad rows instead of raising, ideally logging the
offending row so the rest of the batch continues processing.
In `@app/templates/amendment_pdf.html`:
- Around line 2-4: Add document metadata in amendment_pdf.html by updating the
root html element with a lang attribute and adding a title element in the head.
Use the existing template structure around the html/head tags so WeasyPrint can
pick up the PDF title and language automatically, and make sure the title
reflects the amendment document name used elsewhere in the template.
In `@app/templates/sign.html`:
- Line 32: The Enact button in sign.html is only disabled after the fetch
succeeds, which allows rapid double-clicks to submit duplicate sign requests.
Update the click handling around the Enact button and the sign submit flow so
the button is disabled immediately when the user clicks, before starting the
fetch, and keep it disabled while the request is in flight. Use the existing
Enact button and the sign submission logic in the template to locate the code,
and make sure the disable happens before any POST /sign request is sent.
In `@Dockerfile`:
- Around line 1-14: The Dockerfile currently runs the app as root because there
is no USER set. Add a non-root user in the image, make sure the app files and
working directory are owned or readable by that user, and switch to it before
the CMD so the uvicorn process runs under the non-root account.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 548e075d-79bc-4ae3-a0a0-5e0ebe7a0a28
⛔ Files ignored due to path filters (1)
app/static/signature_pad.umd.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (30)
.dockerignore.github/workflows/ci.yml.github/workflows/image.ymlDockerfileapp/__init__.pyapp/amendments.pyapp/config.pyapp/drive.pyapp/extraction.pyapp/google_clients.pyapp/main.pyapp/pdf.pyapp/pricing.pyapp/static/skipta.cssapp/templates/amendment_pdf.htmlapp/templates/index.htmlapp/templates/sign.htmlpyproject.tomlscripts/verify_access.pytests/__init__.pytests/conftest.pytests/test_amendments.pytests/test_config.pytests/test_drive.pytests/test_extraction.pytests/test_pdf.pytests/test_pdf_html.pytests/test_pricing.pytests/test_routes_intake.pytests/test_routes_sign.py
| permissions: | ||
| contents: read | ||
| packages: write |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Scope packages: write and pin third-party actions.
packages: write is granted at the workflow level for the whole job even though only docker/build-push-action needs it; combined with unpinned action refs (@v4/@v3/@v6 tags rather than commit SHAs), a compromised action release could push under this token's write scope. Consider pinning actions to SHAs and adding a comment justifying the packages: write scope.
Also applies to: 12-30
🧰 Tools
🪛 zizmor (1.26.1)
[error] 7-7: overly broad permissions (excessive-permissions): packages: write is overly broad at the workflow level
(excessive-permissions)
[warning] 7-7: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 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 @.github/workflows/image.yml around lines 5 - 7, The workflow grants
packages: write too broadly and uses unpinned third-party actions, so tighten
the token scope and secure the action refs. In the image workflow, keep
packages: write only where it is needed for docker/build-push-action, add a
clear comment justifying that permission, and replace the `@v4/`@v3/@v6 references
in the workflow steps with pinned commit SHAs for each action such as checkout,
setup-buildx, login, and build-push.
Source: Linters/SAST tools
| found = amendments.find_amendment(sheets, settings.spreadsheet_id, amendment_id) | ||
| if found is None: | ||
| raise HTTPException(status_code=404, detail="Unknown amendment") | ||
| row, record = found | ||
| if record.status == "signed": | ||
| raise HTTPException(status_code=409, detail="Amendment already signed") | ||
|
|
||
| signed_at = datetime.now(timezone.utc) | ||
| record.signed_at = signed_at.isoformat() | ||
| items = json.loads(record.line_items_json) | ||
| html = render_amendment_html(record, items, crew_signature=body.crew_signature_base64, customer_signature=body.customer_signature_base64) | ||
| try: | ||
| pdf_bytes = render_pdf(html) | ||
| except OSError as exc: | ||
| raise HTTPException(status_code=502, detail=f"PDF rendering unavailable: {exc}") from exc | ||
|
|
||
| created_ts = amendment_id.rsplit("_", 1)[-1] | ||
| filename = f"{record.customer_name.replace(' ', '_')}_Amendment_{created_ts}.pdf" # deterministic per amendment — a retry reuses the same name | ||
| try: | ||
| folder_id = drive_mod.ensure_customer_folder(drive, settings.drive_folder_id, record.customer_name) | ||
| pdf_url = drive_mod.find_file_in_folder(drive, folder_id, filename) or drive_mod.upload_pdf(drive, folder_id, filename, pdf_bytes) | ||
| amendments.mark_signed(sheets, settings.spreadsheet_id, row, pdf_url, record.signed_at) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Potential double-sign race on rapid duplicate submissions.
find_amendment → status check → render/upload → mark_signed is a check-then-act sequence with no locking. If two /sign requests for the same amendment_id land concurrently (e.g. a double-click before the UI disables the button, see app/templates/sign.html), both can pass the status == "signed" check, both render+upload a PDF (Drive doesn't enforce filename uniqueness), and both call mark_signed. The Sheets row ends up consistent, but a stray duplicate PDF can be left in Drive.
Pairing this with disabling the "Enact" button immediately on click (see sign.html comment) removes the common trigger; a stronger fix would be an atomic conditional write (e.g., re-check status right before mark_signed, or use the row's existing value as an optimistic-lock precondition).
🤖 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 `@app/main.py` around lines 169 - 190, The sign flow in the amendment handler
has a check-then-act race: `find_amendment`, the `record.status == "signed"`
guard, and the later `mark_signed` call can be hit concurrently, allowing
duplicate PDF uploads for the same `amendment_id`. Fix this by adding an atomic
re-check or optimistic-lock style precondition immediately before
`amendments.mark_signed` in the same handler, and if the row is already signed,
skip the Drive upload path and return the conflict response. Also make sure the
UI-side double-click protection in `sign.html` is paired with this server-side
safeguard.
| def parse_panels(rows): | ||
| return [ | ||
| {"panel_id": r[0], "max_amperage": int(r[1]), "description": r[2], "unit_cost": float(r[3])} | ||
| for r in rows | ||
| if len(r) >= 4 | ||
| ] | ||
|
|
||
|
|
||
| def parse_breakers(rows): | ||
| return [ | ||
| {"breaker_id": r[0], "amps": int(r[1]), "poles": int(r[2]), "description": r[3], "unit_cost": float(r[4])} | ||
| for r in rows | ||
| if len(r) >= 5 | ||
| ] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded numeric parsing of sheet rows can crash the whole pricing path.
int(r[1])/float(r[3]) (and the breaker equivalents) will raise ValueError on any malformed cell — e.g. a currency-formatted price like "$245.00", stray whitespace, or an accidentally non-numeric row in the Panels/Breakers tabs. Since these tabs are hand-edited configuration data (not application-controlled), a single bad row breaks every subsequent amendment-creation request with an unhandled 500, rather than being isolated to that row.
Consider skipping and logging malformed rows instead of letting the whole batch fail:
🛡️ Proposed fix to isolate malformed rows
def parse_panels(rows):
- return [
- {"panel_id": r[0], "max_amperage": int(r[1]), "description": r[2], "unit_cost": float(r[3])}
- for r in rows
- if len(r) >= 4
- ]
+ panels = []
+ for r in rows:
+ if len(r) < 4:
+ continue
+ try:
+ panels.append({"panel_id": r[0], "max_amperage": int(r[1]), "description": r[2], "unit_cost": float(r[3])})
+ except ValueError:
+ logger.warning("skipping malformed panel row: %r", r)
+ return panels🤖 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 `@app/pricing.py` around lines 28 - 41, The row parsers in parse_panels and
parse_breakers currently assume every numeric cell is clean, so a single
malformed sheet row can crash the entire pricing flow. Update these helpers to
validate and safely parse the numeric fields (panel max_amperage/unit_cost,
breaker amps/poles/unit_cost) and skip bad rows instead of raising, ideally
logging the offending row so the rest of the batch continues processing.
| <canvas class="sig" id="crew"></canvas> | ||
| <label>Customer signature <a class="clear" href="#" data-clear="customer">clear</a></label> | ||
| <canvas class="sig" id="customer"></canvas> | ||
| <button id="enact">Enact</button> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Disable the "Enact" button immediately on click, before the fetch resolves.
The button is only disabled after a successful response (Line 55). A rapid double-click sends two concurrent POST /sign requests for the same amendment before the first completes, which is the practical trigger for the backend TOCTOU race flagged in app/main.py (sign_amendment, Lines 169-190).
🔒 Suggested fix
document.getElementById("enact").addEventListener("click", async () => {
const result = document.getElementById("result");
if (pads.crew.isEmpty() || pads.customer.isEmpty()) { result.textContent = "Both signatures are required."; return; }
+ document.getElementById("enact").disabled = true;
result.textContent = "Generating PDF…";
const resp = await fetch("/api/v1/amendments/{{ record.amendment_id }}/sign", {
method: "POST", headers: {"Content-Type": "application/json"},
body: JSON.stringify({ crew_signature_base64: pads.crew.toDataURL(), customer_signature_base64: pads.customer.toDataURL() }),
});
- if (!resp.ok) { result.textContent = "Failed: " + (await resp.text()); return; }
+ if (!resp.ok) { result.textContent = "Failed: " + (await resp.text()); document.getElementById("enact").disabled = false; return; }
const data = await resp.json();
result.innerHTML = `Signed! <a href="${data.pdf_drive_url}">PDF in Drive</a>`;
- document.getElementById("enact").disabled = true;
});Also applies to: 44-56
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 32-32: The type attribute must be present on elements.
(button-type-require)
🤖 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 `@app/templates/sign.html` at line 32, The Enact button in sign.html is only
disabled after the fetch succeeds, which allows rapid double-clicks to submit
duplicate sign requests. Update the click handling around the Enact button and
the sign submit flow so the button is disabled immediately when the user clicks,
before starting the fetch, and keep it disabled while the request is in flight.
Use the existing Enact button and the sign submission logic in the template to
locate the code, and make sure the disable happens before any POST /sign request
is sent.
| FROM python:3.11-slim | ||
|
|
||
| # WeasyPrint runtime libs + a real font for PDF output | ||
| RUN apt-get update && apt-get install -y --no-install-recommends \ | ||
| libpango-1.0-0 libpangoft2-1.0-0 libgdk-pixbuf-2.0-0 fonts-dejavu-core \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| WORKDIR /app | ||
| COPY requirements.txt . | ||
| RUN pip install --no-cache-dir -r requirements.txt | ||
|
|
||
| COPY app/ app/ | ||
| EXPOSE 8000 | ||
| CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8000"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Run container as non-root.
No USER instruction is set, so the process runs as root inside the container. Add a non-root user before the CMD.
🔒 Suggested fix
COPY app/ app/
+RUN useradd -m appuser
+USER appuser
EXPOSE 8000
CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8000"]📝 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.
| FROM python:3.11-slim | |
| # WeasyPrint runtime libs + a real font for PDF output | |
| RUN apt-get update && apt-get install -y --no-install-recommends \ | |
| libpango-1.0-0 libpangoft2-1.0-0 libgdk-pixbuf-2.0-0 fonts-dejavu-core \ | |
| && rm -rf /var/lib/apt/lists/* | |
| WORKDIR /app | |
| COPY requirements.txt . | |
| RUN pip install --no-cache-dir -r requirements.txt | |
| COPY app/ app/ | |
| EXPOSE 8000 | |
| CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8000"] | |
| FROM python:3.11-slim | |
| # WeasyPrint runtime libs + a real font for PDF output | |
| RUN apt-get update && apt-get install -y --no-install-recommends \ | |
| libpango-1.0-0 libpangoft2-1.0-0 libgdk-pixbuf-2.0-0 fonts-dejavu-core \ | |
| && rm -rf /var/lib/apt/lists/* | |
| WORKDIR /app | |
| COPY requirements.txt . | |
| RUN pip install --no-cache-dir -r requirements.txt | |
| COPY app/ app/ | |
| RUN useradd -m appuser | |
| USER appuser | |
| EXPOSE 8000 | |
| CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8000"] |
🧰 Tools
🪛 Checkov (3.3.2)
[low] 1-14: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[low] 1-14: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.14.0)
[warning] 4-4: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[info] 1-1: No HEALTHCHECK defined
Add HEALTHCHECK instruction in your Dockerfile
Rule: DS-0026
(IaC/Dockerfile)
🤖 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 `@Dockerfile` around lines 1 - 14, The Dockerfile currently runs the app as
root because there is no USER set. Add a non-root user in the image, make sure
the app files and working directory are owned or readable by that user, and
switch to it before the CMD so the uvicorn process runs under the non-root
account.
Source: Linters/SAST tools
Names the component properly: skipta (exchanging/shifting between states) and skipta máli (to make a difference, alter the matter) — both apt for a service whose whole job is trading scope mid-job and shifting an amendment draft → signed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@k8s/base/deployment.yaml`:
- Around line 17-18: The deployment currently points to
ghcr.io/siliconsaga/skipta:latest with imagePullPolicy: Always, which makes
releases mutable and non-reproducible. Update the deployment manifest to use a
pinned image reference instead, ideally the CI-published short-SHA tag or an
image digest, and keep the tag updated through CI or kustomize image overrides
rather than tracking latest. Use the image field in the deployment spec as the
location to make this change.
- Around line 13-42: The deployment spec for the skipta container is missing a
hardened securityContext. Update the pod/container definition in the deployment
manifest to add security settings such as runAsNonRoot, allowPrivilegeEscalation
disabled, readOnlyRootFilesystem, and dropped capabilities, and apply them in
the appropriate pod/container securityContext blocks for the skipta container.
If the app needs writable paths at runtime, add the necessary writable volume
mounts so read-only root filesystem remains compatible.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6778cd73-fdf1-4427-9f1c-0c29ae733a84
📒 Files selected for processing (7)
k8s/base/configmap.yamlk8s/base/deployment.yamlk8s/base/httproute.yamlk8s/base/kustomization.yamlk8s/base/namespace.yamlk8s/base/service.yamlk8s/base/serviceaccount.yaml
| spec: | ||
| serviceAccountName: skipta-sa | ||
| containers: | ||
| - name: skipta | ||
| image: ghcr.io/siliconsaga/skipta:latest | ||
| imagePullPolicy: Always | ||
| ports: | ||
| - containerPort: 8000 | ||
| env: | ||
| - name: GCP_PROJECT_ID | ||
| valueFrom: {configMapKeyRef: {name: skipta-config, key: gcp_project_id}} | ||
| - name: GCP_REGION | ||
| valueFrom: {configMapKeyRef: {name: skipta-config, key: gcp_region}} | ||
| - name: SKIPTA_SPREADSHEET_ID | ||
| valueFrom: {configMapKeyRef: {name: skipta-config, key: spreadsheet_id}} | ||
| - name: SKIPTA_DRIVE_FOLDER_ID | ||
| valueFrom: {configMapKeyRef: {name: skipta-config, key: drive_folder_id}} | ||
| - name: SKIPTA_BASE_URL | ||
| valueFrom: {configMapKeyRef: {name: skipta-config, key: base_url}} | ||
| readinessProbe: | ||
| httpGet: {path: /healthz, port: 8000} | ||
| initialDelaySeconds: 3 | ||
| periodSeconds: 5 | ||
| livenessProbe: | ||
| httpGet: {path: /healthz, port: 8000} | ||
| initialDelaySeconds: 15 | ||
| periodSeconds: 10 | ||
| resources: | ||
| requests: {cpu: 250m, memory: 512Mi} | ||
| limits: {cpu: 500m, memory: 1Gi} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a container/pod securityContext.
Static analysis (Trivy/Checkov) flags the container as running with the default security context: no allowPrivilegeEscalation: false, runAsNonRoot: true, readOnlyRootFilesystem: true, or capability dropping. Given this is a public-facing internet service, hardening the container reduces blast radius if compromised.
🔒 Proposed securityContext hardening
spec:
serviceAccountName: skipta-sa
+ securityContext:
+ runAsNonRoot: true
+ seccompProfile: {type: RuntimeDefault}
containers:
- name: skipta
image: ghcr.io/siliconsaga/skipta:latest
imagePullPolicy: Always
+ securityContext:
+ allowPrivilegeEscalation: false
+ readOnlyRootFilesystem: true
+ capabilities: {drop: ["ALL"]}
ports:
- containerPort: 8000Note: readOnlyRootFilesystem: true may require mounting writable emptyDir volumes for WeasyPrint temp files/cache if the app writes to disk at runtime.
📝 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.
| spec: | |
| serviceAccountName: skipta-sa | |
| containers: | |
| - name: skipta | |
| image: ghcr.io/siliconsaga/skipta:latest | |
| imagePullPolicy: Always | |
| ports: | |
| - containerPort: 8000 | |
| env: | |
| - name: GCP_PROJECT_ID | |
| valueFrom: {configMapKeyRef: {name: skipta-config, key: gcp_project_id}} | |
| - name: GCP_REGION | |
| valueFrom: {configMapKeyRef: {name: skipta-config, key: gcp_region}} | |
| - name: SKIPTA_SPREADSHEET_ID | |
| valueFrom: {configMapKeyRef: {name: skipta-config, key: spreadsheet_id}} | |
| - name: SKIPTA_DRIVE_FOLDER_ID | |
| valueFrom: {configMapKeyRef: {name: skipta-config, key: drive_folder_id}} | |
| - name: SKIPTA_BASE_URL | |
| valueFrom: {configMapKeyRef: {name: skipta-config, key: base_url}} | |
| readinessProbe: | |
| httpGet: {path: /healthz, port: 8000} | |
| initialDelaySeconds: 3 | |
| periodSeconds: 5 | |
| livenessProbe: | |
| httpGet: {path: /healthz, port: 8000} | |
| initialDelaySeconds: 15 | |
| periodSeconds: 10 | |
| resources: | |
| requests: {cpu: 250m, memory: 512Mi} | |
| limits: {cpu: 500m, memory: 1Gi} | |
| spec: | |
| serviceAccountName: skipta-sa | |
| securityContext: | |
| runAsNonRoot: true | |
| seccompProfile: {type: RuntimeDefault} | |
| containers: | |
| - name: skipta | |
| image: ghcr.io/siliconsaga/skipta:latest | |
| imagePullPolicy: Always | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| readOnlyRootFilesystem: true | |
| capabilities: {drop: ["ALL"]} | |
| ports: | |
| - containerPort: 8000 | |
| env: | |
| - name: GCP_PROJECT_ID | |
| valueFrom: {configMapKeyRef: {name: skipta-config, key: gcp_project_id}} | |
| - name: GCP_REGION | |
| valueFrom: {configMapKeyRef: {name: skipta-config, key: gcp_region}} | |
| - name: SKIPTA_SPREADSHEET_ID | |
| valueFrom: {configMapKeyRef: {name: skipta-config, key: spreadsheet_id}} | |
| - name: SKIPTA_DRIVE_FOLDER_ID | |
| valueFrom: {configMapKeyRef: {name: skipta-config, key: drive_folder_id}} | |
| - name: SKIPTA_BASE_URL | |
| valueFrom: {configMapKeyRef: {name: skipta-config, key: base_url}} | |
| readinessProbe: | |
| httpGet: {path: /healthz, port: 8000} | |
| initialDelaySeconds: 3 | |
| periodSeconds: 5 | |
| livenessProbe: | |
| httpGet: {path: /healthz, port: 8000} | |
| initialDelaySeconds: 15 | |
| periodSeconds: 10 | |
| resources: | |
| requests: {cpu: 250m, memory: 512Mi} | |
| limits: {cpu: 500m, memory: 1Gi} |
🧰 Tools
🪛 Checkov (3.3.2)
[medium] 1-42: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[low] 1-42: Apply security context to your containers
(CKV_K8S_30)
[low] 1-42: Minimize the admission of containers with the NET_RAW capability
(CKV_K8S_28)
[low] 1-42: Image should use digest
(CKV_K8S_43)
[low] 1-42: Image Tag should be fixed - not latest or blank
(CKV_K8S_14)
[low] 1-42: Minimize the admission of containers with capabilities assigned
(CKV_K8S_37)
[low] 1-42: Apply security context to your pods and containers
(CKV_K8S_29)
[low] 1-42: Use read-only filesystem for containers where possible
(CKV_K8S_22)
[medium] 1-42: Minimize the admission of root containers
(CKV_K8S_23)
[low] 1-42: Containers should run as a high UID to avoid host conflict
(CKV_K8S_40)
[low] 1-42: Ensure that the seccomp profile is set to docker/default or runtime/default
(CKV_K8S_31)
[low] 1-42: Ensure that Service Account Tokens are only mounted where necessary
(CKV_K8S_38)
🪛 Trivy (0.69.3)
[warning] 16-42: Can elevate its own privileges
Container 'skipta' of Deployment 'skipta' should set 'securityContext.allowPrivilegeEscalation' to false
Rule: KSV-0001
(IaC/Kubernetes)
[info] 16-42: Default capabilities: some containers do not drop all
Container 'skipta' of Deployment 'skipta' should add 'ALL' to 'securityContext.capabilities.drop'
Rule: KSV-0003
(IaC/Kubernetes)
[info] 16-42: Default capabilities: some containers do not drop any
Container 'skipta' of 'deployment' 'skipta' in 'skipta' namespace should set securityContext.capabilities.drop
Rule: KSV-0004
(IaC/Kubernetes)
[warning] 16-42: Runs as root user
Container 'skipta' of Deployment 'skipta' should set 'securityContext.runAsNonRoot' to true
Rule: KSV-0012
(IaC/Kubernetes)
[warning] 16-42: Image tag ":latest" used
Container 'skipta' of Deployment 'skipta' should specify an image tag
Rule: KSV-0013
(IaC/Kubernetes)
[error] 16-42: Root file system is not read-only
Container 'skipta' of Deployment 'skipta' should set 'securityContext.readOnlyRootFilesystem' to true
Rule: KSV-0014
(IaC/Kubernetes)
[info] 16-42: Runs with UID <= 10000
Container 'skipta' of Deployment 'skipta' should set 'securityContext.runAsUser' > 10000
Rule: KSV-0020
(IaC/Kubernetes)
[info] 16-42: Runs with GID <= 10000
Container 'skipta' of Deployment 'skipta' should set 'securityContext.runAsGroup' > 10000
Rule: KSV-0021
(IaC/Kubernetes)
[info] 16-42: Runtime/Default Seccomp profile not set
Either Pod or Container should set 'securityContext.seccompProfile.type' to 'RuntimeDefault'
Rule: KSV-0030
(IaC/Kubernetes)
[warning] 16-42: Seccomp policies disabled
container "skipta" of deployment "skipta" in "skipta" namespace should specify a seccomp profile
Rule: KSV-0104
(IaC/Kubernetes)
[info] 16-42: Container capabilities must only include NET_BIND_SERVICE
container should drop all
Rule: KSV-0106
(IaC/Kubernetes)
[error] 16-42: Default security context configured
container skipta in skipta namespace is using the default security context
Rule: KSV-0118
(IaC/Kubernetes)
[error] 13-42: Default security context configured
deployment skipta in skipta namespace is using the default security context, which allows root privileges
Rule: KSV-0118
(IaC/Kubernetes)
[warning] 16-42: Restrict container images to trusted registries
Container skipta in deployment skipta (namespace: skipta) uses an image from an untrusted registry.
Rule: KSV-0125
(IaC/Kubernetes)
🤖 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 `@k8s/base/deployment.yaml` around lines 13 - 42, The deployment spec for the
skipta container is missing a hardened securityContext. Update the pod/container
definition in the deployment manifest to add security settings such as
runAsNonRoot, allowPrivilegeEscalation disabled, readOnlyRootFilesystem, and
dropped capabilities, and apply them in the appropriate pod/container
securityContext blocks for the skipta container. If the app needs writable paths
at runtime, add the necessary writable volume mounts so read-only root
filesystem remains compatible.
Source: Linters/SAST tools
| image: ghcr.io/siliconsaga/skipta:latest | ||
| imagePullPolicy: Always |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Avoid :latest tag with imagePullPolicy: Always.
Static analysis flags this (KSV-0013/CKV_K8S_14): using a mutable tag makes deploys non-reproducible and rollbacks harder — the manifest can't pin to a known-good version. The PR objectives mention CI publishes both :latest and :<short-sha>; consider pinning the deployment to the sha tag (or a digest) and bumping it via CI/kustomize image overrides rather than tracking :latest.
🤖 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 `@k8s/base/deployment.yaml` around lines 17 - 18, The deployment currently
points to ghcr.io/siliconsaga/skipta:latest with imagePullPolicy: Always, which
makes releases mutable and non-reproducible. Update the deployment manifest to
use a pinned image reference instead, ideally the CI-published short-SHA tag or
an image digest, and keep the tag updated through CI or kustomize image
overrides rather than tracking latest. Use the image field in the deployment
spec as the location to make this change.
Source: Linters/SAST tools
… exact-match Drive folder lookup, PDF title/lang Three accepted findings from the PR #1 CodeRabbit/Copilot round (16 others triaged as reject/defer/known — rebuttals on their threads): - google-cloud-aiplatform capped at <1.160: vertexai.generative_models is past its announced removal date, so an uncapped fresh image build could ImportError on the first extraction call while local envs keep working. 1.159 verified locally to ship the module; today's CI resolution also imports it in the extraction tests. - Drive folder lookup uses name = instead of name contains — prefix matching could file Smith's signed PDF into Smithson's folder. Exact-match operator now pinned by test. - amendment_pdf.html gains lang + a title so generated PDFs carry real /Title metadata. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cervator
left a comment
There was a problem hiding this comment.
Merging early since the rabbit seems caught up in throttling land and this is an MVP only anyway
Summary
docs/plans/2026-07-01-skipta-field-amendments-{design,plan}.md): voice-text change order → Gemini structured extraction (Vertex AI, model fallback chain) → deterministic Sheets pricing with an UNMATCHED guard against invented parts → server-rendered signing page (vendored signature_pad, no CDN) → WeasyPrint-flattened PDF → customer's Drive folder, with the Amendments sheet tab as the draft→signed state machine.skipta-sa→skipta-gsa), ADC impersonation locally; Drive/Sheets access via asset sharing with the GSA.mark_signedonly after a Drive URL exists, honest 502s on upstream failure; 404/409/422 contracts tested.amend__…ids.ci(ruff + full pytest incl. the real PDF render on ubuntu) andimage(ghcr, sha+latest, gha cache — ting's pattern). k8s kustomize base + guarded deploy follow as the next commit on this branch after the ConfigMap IDs land.ci).Test plan
ciworkflow green on this PR (ruff + 33-test suite including the real WeasyPrint PDF render).imageworkflow publishesghcr.io/siliconsaga/skipta:latest; live e2e smoke athttps://skipta.cmdbee.org(create → sign → PDF in Drive → row flipssigned) tracked in the plan's Task 15.Related
docs/plans/in this repo.