Read the trial and the access state the cloud sends, and render them#77
Read the trial and the access state the cloud sends, and render them#77Coding-Dev-Tools wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2904e9889d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR fixes three intertwined bugs where
Confidence Score: 4/5Safe to merge; the core plumbing is correct and well-tested with 37 targeted new tests. The Python logic in Files Needing Attention: engraphis/static/dashboard.js — the escaping contract on
|
| Filename | Overview |
|---|---|
| engraphis/cloud_session.py | Extends _declared_entitlement to persist status, is_trial, trial_consumed, and trial_ends_at from cloud responses; widens the plan-change key-drop logic from cloud_features-only to all _ENTITLEMENT_KEYS; clears status (but correctly preserves trial facts) on billing denial. Logic is sound with well-considered edge cases. |
| engraphis/routes/v2_api.py | Introduces _trial_facts, _epoch_seconds, _normalized_status, _access_state, and _resolved_entitlement helpers; threads the five new fields through all four entitlement sources (session, cache, auth fetch, fallback); replaces hardcoded is_trial=False/trial.used=False in /api/license. Python 3.9 Z-suffix workaround and bool-before-int guard in _epoch_seconds are correct. |
| engraphis/static/dashboard.js | Adds licAccessState/licTrialAvailable/lockReason and rewires badge, upgrade panel, and Team teaser copy to the access state. Two P2 findings: lockReason() has an inconsistent escaping contract (direct embed in unlockHtml vs esc()-wrapped in loadOverviewAnalytics), and lockReason(true) in loadTeam emits misleading copy for active Team subscribers. |
| engraphis/static/dashboard.css | Adds three CSS classes (.lic-banner, .lic-banner strong, .lic-banner-warn) for the new state banners; no inline styles or security-relevant changes. |
| tests/test_hosted_plan_resolution.py | 37 new tests covering every access state, trial-field round-trip through session/cache, billing-denial interaction, Python 3.9 timestamp parsing, and drift-guard assertions on JS asset patterns; each explicitly constructed to fail on origin/main. |
| tests/test_dashboard_auth_placement.py | Expands the node harness to bundle the real access-state readers instead of stubs, and adds a parametrised case verifying the upgrade panel never offers a trial in states where start_trial would 409. |
| tests/test_client_launch.py | Extends the payload contract test to assert access_state, entitlement_status, and the four new trial sub-fields (active, available, ends_at, trial_days) are present on /api/license. |
Sequence Diagram
sequenceDiagram
participant CP as Control Plane
participant CS as cloud_session.py
participant API as v2_api.py /api/license
participant DB as dashboard.js
CP->>CS: register/refresh (plan, is_trial, trial_consumed, trial_ends_at, status)
CS->>CS: "_declared_entitlement() — validate & cap fields"
CS->>CS: _save() → session record
DB->>API: GET /api/license
API->>CS: saved_entitlement()
CS-->>API: "session dict (or {})"
API->>API: _session_entitlement() → _trial_facts(session)
API->>API: "_access_state(entitlement) → active|trial|trial_expired|lapsed|inactive"
API-->>DB: "{plan, access_state, is_trial, trial:{used, active, available, ends_at}, ...}"
alt billing denial (402)
CP-->>CS: record_billing_denial()
CS->>CS: "cloud_access_active=False, pop status, keep is_trial/trial_ends_at"
API->>API: _deny_entitlement_cache() — status empty, trial facts survive
Note over API,DB: access_state flips: trial→trial_expired, active→lapsed
end
DB->>DB: licAccessState() → read access_state
DB->>DB: updateLicBadge() — TRIAL/TRIAL ENDED/PRO INACTIVE/PRO/LOCAL
DB->>DB: licStateBanner() — warning banner per state
DB->>DB: licActionsHtml() — trial CTAs only when licTrialAvailable()
Comments Outside Diff (2)
-
engraphis/static/dashboard.js, line 552-559 (link)Inconsistent escaping contract on
lockReason()return valuelockReason()escapes its dynamic parts internally withesc()(forendsandlicPlanName()), making its return value a plain-text string that happens to be HTML-safe. However, it is used in two incompatible ways: inunlockHtmlthe result is embedded directly into a template literal (${detail}) as if it were trusted HTML, while inloadOverviewAnalyticsit is wrapped again inesc()producing double-escaping. Both work today because all dynamic values (ends→"YYYY-MM-DD",licPlanName()→"PRO"/"TEAM"/"LOCAL") are ASCII-only with no HTML special characters. But the contract is ambiguous: if a future change adds a user-controlled value and escapes it insidelockReason(), theunlockHtmlpath stays safe while theloadOverviewAnalyticspath silently double-escapes it — or the reverse if the author forgets the inneresc(). Picking one contract (return HTML, no outeresc()needed; or return plain text, always wrap withesc()at the call site) and applying it consistently would make the right path obvious.Prompt To Fix With AI
This is a comment left during a code review. Path: engraphis/static/dashboard.js Line: 552-559 Comment: **Inconsistent escaping contract on `lockReason()` return value** `lockReason()` escapes its dynamic parts internally with `esc()` (for `ends` and `licPlanName()`), making its return value a plain-text string that happens to be HTML-safe. However, it is used in two incompatible ways: in `unlockHtml` the result is embedded directly into a template literal (`${detail}`) as if it were trusted HTML, while in `loadOverviewAnalytics` it is wrapped again in `esc()` producing double-escaping. Both work today because all dynamic values (`ends` → `"YYYY-MM-DD"`, `licPlanName()` → `"PRO"/"TEAM"/"LOCAL"`) are ASCII-only with no HTML special characters. But the contract is ambiguous: if a future change adds a user-controlled value and escapes it inside `lockReason()`, the `unlockHtml` path stays safe while the `loadOverviewAnalytics` path silently double-escapes it — or the reverse if the author forgets the inner `esc()`. Picking one contract (return HTML, no outer `esc()` needed; or return plain text, always wrap with `esc()` at the call site) and applying it consistently would make the right path obvious. How can I resolve this? If you propose a fix, please make it concise.
-
engraphis/static/dashboard.js, line 640 (link)lockReason(true)is called unconditionally inloadTeam, regardless of the customer's access state. WhenlicAccessState()returns'active'for a customer already on a Team subscription,lockReason(true)returns"Your TEAM subscription does not include this."— which contradicts reality. The Team Cloud card is a teaser pointing to the hosted service, and even paying Team subscribers use it to open Team Cloud, so the limitation being described is the local dashboard not including hosted team management — not a subscription deficiency. The copy is accurate for Pro subscribers and trialists but incorrect for active Team subscribers who already paid for exactly what the card advertises. Guarding thelockReason()call with a plan check (e.g., only render the "does not include" line whenlicPlanName() !== 'TEAM') or using a separate fixed hint string in theactivecase would fix it.Prompt To Fix With AI
This is a comment left during a code review. Path: engraphis/static/dashboard.js Line: 640 Comment: **Misleading copy for Team subscribers in the Team Cloud teaser** `lockReason(true)` is called unconditionally in `loadTeam`, regardless of the customer's access state. When `licAccessState()` returns `'active'` for a customer already on a Team subscription, `lockReason(true)` returns `"Your TEAM subscription does not include this."` — which contradicts reality. The Team Cloud card is a teaser pointing to the hosted service, and even paying Team subscribers use it to open Team Cloud, so the limitation being described is the local dashboard not including hosted team management — not a subscription deficiency. The copy is accurate for Pro subscribers and trialists but incorrect for active Team subscribers who already paid for exactly what the card advertises. Guarding the `lockReason()` call with a plan check (e.g., only render the "does not include" line when `licPlanName() !== 'TEAM'`) or using a separate fixed hint string in the `active` case would fix it. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
engraphis/static/dashboard.js:552-559
**Inconsistent escaping contract on `lockReason()` return value**
`lockReason()` escapes its dynamic parts internally with `esc()` (for `ends` and `licPlanName()`), making its return value a plain-text string that happens to be HTML-safe. However, it is used in two incompatible ways: in `unlockHtml` the result is embedded directly into a template literal (`${detail}`) as if it were trusted HTML, while in `loadOverviewAnalytics` it is wrapped again in `esc()` producing double-escaping. Both work today because all dynamic values (`ends` → `"YYYY-MM-DD"`, `licPlanName()` → `"PRO"/"TEAM"/"LOCAL"`) are ASCII-only with no HTML special characters. But the contract is ambiguous: if a future change adds a user-controlled value and escapes it inside `lockReason()`, the `unlockHtml` path stays safe while the `loadOverviewAnalytics` path silently double-escapes it — or the reverse if the author forgets the inner `esc()`. Picking one contract (return HTML, no outer `esc()` needed; or return plain text, always wrap with `esc()` at the call site) and applying it consistently would make the right path obvious.
### Issue 2 of 2
engraphis/static/dashboard.js:640
**Misleading copy for Team subscribers in the Team Cloud teaser**
`lockReason(true)` is called unconditionally in `loadTeam`, regardless of the customer's access state. When `licAccessState()` returns `'active'` for a customer already on a Team subscription, `lockReason(true)` returns `"Your TEAM subscription does not include this."` — which contradicts reality. The Team Cloud card is a teaser pointing to the hosted service, and even paying Team subscribers use it to open Team Cloud, so the limitation being described is the local dashboard not including hosted team management — not a subscription deficiency. The copy is accurate for Pro subscribers and trialists but incorrect for active Team subscribers who already paid for exactly what the card advertises. Guarding the `lockReason()` call with a plan check (e.g., only render the "does not include" line when `licPlanName() !== 'TEAM'`) or using a separate fixed hint string in the `active` case would fix it.
Reviews (1): Last reviewed commit: "Read the trial and the access state the ..." | Re-trigger Greptile
2904e98 to
b5f1b4b
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
b5f1b4b to
641bab9
Compare
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 641bab9382
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`/api/license` hardcoded `"is_trial": False` and `"trial": {"used": False}`.
Those were the only assignments in the file, and the client read none of
`status`, `trial_ends_at`, or `trial_duration_seconds` from anywhere, so three
customer-visible states were wrong or unreachable:
* the dashboard's `'TRIAL'` badge branch could never be taken — a trialist saw
the same confident PRO/TEAM badge a subscriber sees;
* `trial.used` never became true, so "Start hosted Pro/Team trial" was offered
to everyone forever, including active subscribers and customers whose trial
was already spent. `start_trial` refuses any organization that already holds
an entitlement, so for every connected customer that button could only ever
return a `TrialAlreadyConsumedError`; and
* `cloud_access_active` was computed, returned, and rendered nowhere. After a
trial expired or a subscription lapsed the client kept `plan="pro"` and
cleared the feature list, so the customer got a confident PRO badge over
rows of locked features with no explanation of what had happened.
The client now reads what the control plane already sends on the calls it
already makes. `cloud_session` persists `status`, `is_trial`,
`trial_consumed`, and `trial_ends_at` from the registration/refresh handshake
alongside the plan fields, under the same wire names so one parser serves the
saved record, the entitlements read, and the compatibility cache. Every field
stays individually optional: an older control plane omits them and the honest
answer is "not a trial, none consumed", never an invented one. A billing
denial keeps the trial facts — a lapse does not un-consume a trial or move its
boundary — and drops the status it just contradicted.
`/api/license` derives one `access_state` from that: active, trial,
trial_expired, lapsed, or inactive. Each is a different thing to tell the
customer and a different thing to ask them to do, and the dashboard now says
which:
* active trial — TRIAL badge, the trial's end date, no trial CTA;
* spent trial — TRIAL ENDED, "your free trial has ended, your memories are
still local and usable, the trial cannot be started again",
subscribe;
* paying — PRO/TEAM badge, no trial CTA (a subscriber who never
trialled would still be refused one);
* lapsed — PRO INACTIVE, why it lapsed when the cloud named a status,
and update billing;
* never — LOCAL CORE, the local engine is complete on its own, and
the trial CTA — the one state it can actually be started in.
`plan_source`/`plan_checked_at` were emitted for support and displayed
nowhere; the panel now shows which rule produced the answer and when the cloud
last confirmed it. No inline styles or handlers were added; both asset gates
and `node --check` pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
641bab9 to
ca97051
Compare
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca97051ba5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Superseded by #80, which consolidated this change with the complete audited 1.1.0 client release and is now merged into main. |
2 participants
Paired with cloud PR: Coding-Dev-Tools/engraphis-cloud#31 — the two must ship together. Do not merge either alone. This client degrades safely against a control plane that predates the new fields (they are each individually optional), but the states below are only reachable once #31 is deployed.
What was broken
engraphis/routes/v2_api.pyhardcoded"is_trial": Falseand"trial": {"used": False, ...}. Those were the only assignments in the file, and nothing readstatus,trial_ends_at, ortrial_duration_secondsfrom anywhere. Three consequences:'TRIAL'badge branch indashboard.jswas unreachable. Trialists saw the same confident PRO/TEAM badge a subscriber sees.usedwas always false, it was offered to active paying subscribers and to customers whose trial was already spent — both of whom the control plane answersTrialAlreadyConsumedErrorfor. The button could only ever produce a 409.cloud_access_activewas computed, returned, and rendered zero times. After a trial expired or a subscription lapsed, the client keptplan="pro"and cleared the features, so the customer saw a confident PRO badge with every feature row locked and no explanation.plan_sourcewas likewise emitted and never displayed.What this changes
Plumbing.
cloud_session._declared_entitlementpersistsstatus,is_trial,trial_consumed, andtrial_ends_atfrom the registration/refresh handshake alongside the plan fields, under the same wire names — so one parser serves the saved session record, theGET /v1/entitlements/{org}answer, and the compatibility cache, and the three cannot be read by different rules. Each field stays individually optional; an older control plane omits them and the honest answer is "not a trial, none consumed", never an invented one. On a plan change, every entitlement key the new answer did not restate is dropped, so a finished trial'sis_trial/trial_ends_atcannot survive under the paid plan it converted into. A billing denial keeps the trial facts (a lapse does not un-consume a trial or move its boundary) and drops the status it just contradicted.One derived
access_state, in Python where it is testable:active | trial | trial_expired | lapsed | inactive./api/licensenow emits it plusentitlement_status, realis_trial, andtrial: {used, active, ends_at, available, trial_days}.A cached
cloud_access_activecannot outlive the trial it describes. That flag is only ever as fresh as the last answer from the control plane, so an installation that goes offline mid-trial keeps reading back thetruesaved while the trial was live._access_statechecks the boundary the server itself disclosed before trusting it, andhosted_plan_summary()reports nofeaturesfor any non-live state — so the panel cannot call a trial live under a printed end date already in the past, nor tick capability rows every hosted call is about to deny.trial.availableis deliberately gated onplan_source == "local"— an installation belonging to no organization.start_trialrefuses every organization that already holds an entitlement, so a connected customer is trialling, has spent one, or is paying, and in all three cases the CTA can only 409. That also covers the one-boot window where the plan is inferred and nothing has answered yet.What the dashboard renders now
TRIAL(accent)TRIAL ENDED(muted)PRO/TEAM(accent)PRO INACTIVE(muted)past_due→ "the last payment did not go through",canceled,expired,revoked). "Your local memories are unaffected."LOCAL CORE(muted)The same
lockReason()drives the Analytics/Automation upgrade panels, the overview analytics teaser, and the Team teaser, so no surface can offer a trial another one hides.plan_source/plan_checked_atare finally displayed as a support diagnostic row.Review fixes in this revision
Five findings from the codex review, all confirmed and fixed:
trial_ends_at,access_statestayedtrialindefinitely. Now settled from the disclosed boundary, and the non-live feature list is emptied with it.?plan=pro— the wrong product offered to fix a billing problem — and "Open account portal" sent a lapsed Pro customer to the Team checkout. Renewal now followsLIC.plan.LIC.upgrade_url, which is not plan-neutral:hosted_client.upgrade_url(None)resolvesplan="pro"inside and so prefersENGRAPHIS_PRO_UPGRADE_URL. A newhosted_client.account_url()resolves the generic value directly and falls back to the hosted account root rather than to either plan page;/api/licenseemits it asaccount_urland the portal button reads that. Both test harnesses had hidden this by pointingupgrade_urlat a third URL the route can never produce — they now mirror the real resolution.lockReason()is denial copy — every other caller reaches it because a hosted request came back 402/501 — butloadTeam()renders for everyone on every visit, so a live Team subscriber read "Your TEAM subscription does not include this" above an unlocked Team nav item. A newteamTeaserNote()says what is true for a live Team plan (including a Team trial and its boundary) and falls back to the denial copy for everyone it is actually about: a Pro subscriber, a lapsed plan, a spent trial, a free installation.json.loadsturns1e309intoinfand accepts a bareInfinity; either reachedJSONResponse, which raises on non-JSON-compliant floats — from/api/licenseand therefore/api/bootstrap._epoch_secondsnow requiresmath.isfinite, and catches theOverflowErroran arbitrary-precision JSON integer raises on the way tofloat().Rebase
Rebased onto
73aa244(after #73, #76, #78). #76 removedexportfrom the hosted entitlement tables and from the upgrade panel's benefit list, so the duplicate "Signed compliance exports with bi-temporal checksums" line was dropped from this branch'sunlockHtmlrather than reintroduced; nothing else in the conflict was a duplicate of work already on main.CSP
No inline styles or handlers added. Three named CSS classes (
.lic-banner,.lic-banner strong,.lic-banner-warn) indashboard.css; existingdata-csp-style/data-onclicktokens reused.python scripts/externalize_dashboard_assets.pyis a no-op,python scripts/check_commercial_manifest.pypasses, andnode --check engraphis/static/dashboard.jsis clean.Tests
47 new tests, each verified to fail on the revision it guards:
test_hosted_plan_resolution.py— one test per rendered state; the trial arriving on the handshake with no extra request; a denial turning a running trial into an expired one; a cached live trial expiring once its own boundary passes, and surviving a fully offline re-read as expired; a running trial not expired by a boundary that has not arrived or was never disclosed; a non-finite or arbitrary-precision boundary never reaching the JSON encoder; a field-less control plane claiming no trial; the disclosure surviving a restart through the cache; theZ-suffixed UTC timestamp Pydantic emits being readable on the Python 3.9 floor; a forged status not reaching the copy; a drift guard naming the exact server fields; and asset-parsing tests closing the loop on the shipped JS.test_dashboard_auth_placement.py— the real access-state readers are bundled into the node harness rather than stubbed. A parametrised case proves the upgrade panel offers no trial in the trial / trial_expired / lapsed / active states while still offering the purchase, and a second node harness executeslicActionsHtmlagainst three distinct hosted URLs to prove each access state's buttons carry the URL that can actually succeed.test_client_launch.py— the payload contract extended to the new keys.tests/e2e/commercial.spec.js— the Playwright fixture built/api/licensepayloads by hand and so could not produceaccess_state,plan_source, ortrial.availableat all.licenseFor()now mirrorsv2_api.get_license()including the derived keys, which is what a real browser needs in order to reach any of these states. The lapsed-Team case asserts the new copy and that "Update billing" carries the Team checkout, and a new case covers a spent trial end to end (badgeTRIAL ENDED, the dated explanation, Subscribe rather than Start trial). The Team-customer and Pro-customer cases now also assert the Team tab’s copy.Verification
python -m pytest tests/ -q: 1450 passed, 0 failed, 14 skipped (1464 collected).npx playwright test: 12 passed, 0 failed (Chromium, run locally against the real dashboard).ruff check .: clean.eval.harnessonsample.jsonlandcodemem.jsonl@ k=5, pluseval.ablation): green, numbers unchanged.ast.parse(feature_version=(3, 9))guard over the touched modules.Not verified
The states are exercised in a real Chromium against the real dashboard, but with
/api/licensemocked — the control plane itself was never contacted. There are no screenshots and no manual pass. The end-to-end path against a live control plane was not run; the cross-repo contract is pinned by the mirrored field list here and by the parity test in #31.🤖 Generated with Claude Code