Pre-ISAM fixes: photos reach the assistant, staff-only intake, dead button removed - #31
Open
philosophercode wants to merge 2 commits into
Open
philosophercode wants to merge 2 commits into
philosophercode wants to merge 2 commits into
Conversation
Three changes to merged specs, recorded as dated amendments per DRIFT.md: - intake §6.1: attached photos never reached the model. Records the fix and two details the spec left open (earlier photos are bounded; a photo the browser cannot decode still uploads). - intake §4.4: the duplicate card's "add a unit" button is withdrawn, because nothing could act on it. - auth §3.4 and §8: adding equipment is staff-only on the chat surface. The README's drift record gains a section for the three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NP2xC6APXbSucoE4n5mtoj
Photos: ChatFab uploaded each photo to Notion and sent only a text hint, so the model never saw one. The browser now also draws a JPEG copy no larger than 1568px on the long edge and sends it as a file part on the same message. Earlier turns keep at most four photos (withRecentPhotos), and a photo the browser cannot decode still uploads. Staff-only intake: a capability can declare minimumRole, and capabilitiesForRole in capabilities/access.ts enforces it once, when the chat composes its tools. Intake requires staff. Anonymous visitors and students get a one-line explanation instead of the tools, and the header's Add button appears only for staff and admins. MCP is unchanged; MCP_TOKEN remains its gate. Duplicate card: "Add a unit to the existing tool" seeded a message no tool could handle. It is removed, with its label in all 12 locale files, and the prompt now explains how to resolve "No, create a new tool", which it never listed. test:all: lint (0 errors, 3 pre-existing warnings), typecheck, 820 unit and integration tests, 33 E2E. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NP2xC6APXbSucoE4n5mtoj
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: 5045b59988
ℹ️ 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".
|
|
||
| for (let i = messages.length - 1; i >= 0; i -= 1) { | ||
| const message = messages[i]; | ||
| if (message.role !== "user" || i === latestUser) continue; |
There was a problem hiding this comment.
When a user selects several photos, this exemption leaves every image on the newest message plus four older images in the JSON request. The multiple file input has no count or aggregate-byte cap, and resizing to 1568 px does not bound JPEG size; base64 further expands it. Valid batch-identification requests can therefore exceed the deployed platform's request-body ceiling and fail before /api/chat runs. Enforce an aggregate encoded-byte or image-count budget that also covers the latest turn, as the intake spec requires the request-size ceiling to be respected.
AGENTS.md reference: AGENTS.md:L52-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
Photo batching and positional pairing remain unresolved, and one specification coverage link needs correction.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR prepares v5 for the ISAM demo by enabling photo vision delivery, restricting equipment intake to staff/admins, and removing an unsupported duplicate action.
Changes:
- Adds downscaled photo file parts with bounded historical photo retention.
- Enforces staff-only intake and hides the Add button for unauthorized users.
- Removes the duplicate action and obsolete translations, and updates specifications.
Open review items include unbounded latest-message photo batches, positional photo/upload mismatches, and one incorrect specification path.
File summaries
| File | Reviewed change |
|---|---|
v5/src/lib/chat/photo-parts.ts |
Builds photo parts and filters photo history; positional pairing and per-turn limits need correction. |
v5/src/lib/chat/photo-parts.test.ts |
Tests photo-part conversion and retention limits. |
v5/src/lib/chat/downscale-image.ts |
Downscales browser images to JPEG. |
v5/src/lib/chat/downscale-image.test.ts |
Tests image scaling and cleanup. |
v5/src/lib/capabilities/types.ts |
Adds capability role metadata. |
v5/src/lib/capabilities/intake.ts |
Gates intake to staff and removes the dead action. |
v5/src/lib/capabilities/intake.test.ts |
Tests intake behavior and access. |
v5/src/lib/capabilities/index.ts |
Exports access helpers. |
v5/src/lib/capabilities/access.ts |
Filters capabilities by role. |
v5/src/lib/capabilities/access.test.ts |
Tests capability authorization. |
v5/src/lib/auth/identity.ts |
Documents the authorization boundary. |
v5/src/components/PrimaryNav.tsx |
Shows Add only to staff/admins. |
v5/src/components/PrimaryNav.test.tsx |
Tests Add-button visibility. |
v5/src/components/ChatFab.tsx |
Uploads photos and sends vision parts; latest-message limits remain incomplete. |
v5/src/components/ChatFab.test.tsx |
Tests photo transmission and retention. |
v5/src/app/api/chat/route.ts |
Composes role-aware chat capabilities and vision handling. |
v5/src/app/api/chat/route.test.ts |
Tests authorization and vision delivery. |
v5/messages/zh-CN.json |
Removes obsolete action translation. |
v5/messages/tr.json |
Removes obsolete action translation. |
v5/messages/ru.json |
Removes obsolete action translation. |
v5/messages/pt-BR.json |
Removes obsolete action translation. |
v5/messages/ko.json |
Removes obsolete action translation. |
v5/messages/ja.json |
Removes obsolete action translation. |
v5/messages/hi.json |
Removes obsolete action translation. |
v5/messages/he.json |
Removes obsolete action translation. |
v5/messages/fr.json |
Removes obsolete action translation. |
v5/messages/es.json |
Removes obsolete action translation. |
v5/messages/en.json |
Removes obsolete action translation. |
v5/messages/ar.json |
Removes obsolete action translation. |
docs/specs/README.md |
Records the specification amendments. |
docs/specs/2026-07-29-auth-and-rate-limiting-design.md |
Documents staff-only intake; one coverage path needs correction. |
docs/specs/2026-06-01-chat-inventory-intake-design.md |
Documents vision and duplicate-action amendments. |
Review details
Suppressed comments (2)
docs/specs/2026-07-29-auth-and-rate-limiting-design.md:436
- This new coverage reference points to
src/..., but the spec is repository-rooted and the surrounding references usev5/src/...(for example, lines 375 and 425).src/lib/capabilities/access.test.tsis not the path of the added test fromdocs/specs; update the link so the accepted coverage record is navigable.
**Status.** Accepted. Covered by `src/lib/capabilities/access.test.ts`, the chat route tests
v5/src/components/ChatFab.tsx:267
- This bounds only photos from earlier messages, while the latest message keeps every attachment and the file input has no per-turn cap. A batch of high-detail images can therefore exceed the chat/platform request limit despite the 1568px edge limit, and a direct POST can bypass this client-only bound; the
useChatstate also retains every encoded data URL after it is excluded from later requests. Add a byte/photo ceiling before sending and enforce the same limit server-side, with a deliberate strategy for pruning old file bytes.
// Every turn re-sends the conversation; keep earlier photos bounded
// so their bytes do not ride along forever (Article 4).
messages: withRecentPhotos(messages),
- Files reviewed: 32/32 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+25
to
+27
| .filter((photo): photo is VisionPhoto & { dataUrl: string } => | ||
| Boolean(photo.dataUrl) | ||
| ) |
Comment on lines
+41
to
+42
| * user message keeps all of its photos — that is the turn asking about them — | ||
| * and earlier turns keep at most `limit` more, newest first, so a follow-up |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes before the ISAM demo
ChatFabuploaded each photo to Notion and sent only an[Attached photos: …]text hint, so identification and troubleshooting from a photo never worked, although the abstract claims vision. The browser now draws a JPEG copy (longest edge 1568px) alongside the upload and sends it as a file part. Earlier turns keep at most four photos, so requests stay bounded.minimumRole: "staff", enforced once incapabilities/access.tswhen the chat composes its tools. Anonymous visitors and students get a short explanation instead of the tools, and the header's Add button shows only for staff and admins. MCP is unchanged;MCP_TOKENis still its gate.Process note
Constitution Article 1 wants spec changes merged before implementation. These are dated amendments to already-merged specs (intake §4.4 and §6.1, auth §3.4 and §8), so they ride in this PR as its first commit. Flagging it here rather than hiding it.
After merge — needs a person
AUTH_STAFF_EMAILSin Vercel. Until now anyone could add tools; after this, only staff and admins can.RATE_LIMIT_ANON_CHATfor Oct 11. Conference wifi puts every visitor behind one IP.Verification
npm run test:allpasses: lint (0 errors, 3 pre-existing warnings), typecheck, 820 unit and integration tests in 55 files, and 33 E2E.spec:coveragereports 67 items, 0 undocumented.🤖 Generated with Claude Code
https://claude.ai/code/session_01NP2xC6APXbSucoE4n5mtoj