Store user_id as comment author_id for local_agent comments (COPLAN-33)#128
Conversation
API-created comments previously stored the API token's id as author_id, forcing Comment#author to resolve local_agent authors via a JOIN through coplan_api_tokens. Store the resolved user's id instead so #author collapses to a single find_by(id:) for both human and local_agent; agent_name still distinguishes which agent posted. Includes a backfill migration that rewrites existing local_agent author_id values from token ids to user ids (idempotent, re-runnable). Amp-Thread-ID: https://ampcode.com/threads/T-019eada7-54c1-7268-a08a-f461ce3be884 Co-authored-by: Amp <amp@ampcode.com>
![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: c3fdf050a0
ℹ️ 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".
| @@ -0,0 +1,22 @@ | |||
| class BackfillLocalAgentCommentAuthorIds < ActiveRecord::Migration[8.1] | |||
There was a problem hiding this comment.
This backfill is added only under the host app's db/migrate, but the code it supports is in the packaged engine. I checked engine/coplan.gemspec, and the gem only packages files under the engine directory ({app,config,db,lib,prompts}), so upgrading a non-repo host with coplan-engine will get the new Comment#author behavior without receiving this migration; existing local_agent comments whose author_id still points at coplan_api_tokens.id will stop resolving to a user. Please put the migration in engine/db/migrate (and copy it into the host as usual) so engine consumers receive the backfill.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in c870c1d. Moved the backfill to engine/db/migrate/ (mirroring the existing backfill_structured_tags precedent) and copied it into the host via co_plan:install:migrations (db/migrate/...backfill_local_agent_comment_author_ids.co_plan.rb), so gem consumers now receive the backfill alongside the engine Comment#author change.
The coplan-engine gem only packages engine/** files, and the Comment#author change ships in the engine. Hosting the backfill only under the host app's db/migrate meant external consumers would get the new behavior without the data backfill, breaking resolution of existing local_agent comments. Move the migration to engine/db/migrate (mirroring the backfill_structured_tags precedent) and copy it into the host via co_plan:install:migrations. Addresses Codex review feedback. Amp-Thread-ID: https://ampcode.com/threads/T-019eada7-54c1-7268-a08a-f461ce3be884 Co-authored-by: Amp <amp@ampcode.com>
1 participant
Summary
Store the resolved user's id as
author_idforlocal_agentcomments created via the API, instead of the API token's id. This letsComment#authorcollapse to a singlefind_by(id: author_id)for bothhumanandlocal_agentcomments — no JOIN throughcoplan_api_tokens, no hardcoded table name.agent_name(already required for local_agent) still distinguishes which agent posted on the user's behalf.Closes #29
Linear: COPLAN-33
Changes
engine/app/controllers/coplan/api/v1/comments_controller.rb— both comment-create sites (create,reply) now set commentauthor_id: current_user&.idinstead ofapi_actor_id. Lease/session/notification actor semantics (api_actor_id) are unchanged.engine/app/models/coplan/comment.rb—#authorsimplified to a singlefind_by(id: author_id)for bothhumanandlocal_agent(returns nil forcloud_persona/system).db/migrate/20260609000000_backfill_local_agent_comment_author_ids.rb— backfill that rewrites existinglocal_agentcomments whoseauthor_idis acoplan_api_tokens.idto that token'suser_id. Idempotent and safe to re-run (downis a no-op; irreversible by design).Testing
#authorresolves to the user;Comment#authorresolves both types via direct lookup;comment_author_namestill renders "Agent (via User)"; backfill migration rewrites token-id rows, leaves human/already-migrated rows untouched, and is idempotent.bundle exec rspec— all non-system specs pass (845 examples); targeted specs green. System-spec failures seen only under the full suite are pre-existing MySQL deadlock / "table definition changed" flakes from the sharedcoplan_testDB (each passes in isolation), unrelated to this change.bundle exec rubocop— clean on all changed files.