Establish Helpthread’s support infrastructure charter - #112
Conversation
📝 WalkthroughWalkthroughHelpthread’s documentation is reorganized around a v2 charter, explicit governance and legal boundaries, updated architecture records, Agent Inbox and authentication contracts, clarified mail semantics, and revised module marketplace specifications. ChangesProject foundation
Architecture and project records
API and authentication contracts
Mail and conversation contracts
Module catalog and marketplace
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
specs/mail/sending.md (1)
178-200: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRepair the broken identifier and sentence fragments.
The changed text renders
SendReplyInput.fromasSendReplyInput.followed byfrom, and later contains “The fix is...”. Restore the identifier and rewrite the sentence so the self-echo guard contract can be copied accurately by implementers.Also applies to: 203-219
🤖 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 `@specs/mail/sending.md` around lines 178 - 200, In the affected documentation passages, restore the broken identifier as SendReplyInput.from and remove the extra spacing in “The fix is...”. Ensure the surrounding sentence clearly and accurately states the self-echo guard contract for implementers, preserving the existing technical meaning.specs/modules/substrate-v1.md (1)
106-112: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRepair malformed draft and approval prose.
The changed text contains “A draft is inert rows until approval” and “( semantics unchanged”. Rewrite both sentences before treating this as the implementation contract.
Also applies to: 248-253
🤖 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 `@specs/modules/substrate-v1.md` around lines 106 - 112, Rewrite the malformed prose in the schema and delivery-status contract: replace “A draft is inert rows until approval” with grammatically correct wording stating that drafts remain inert until approval, and correct the malformed “( semantics unchanged” parenthetical while preserving the intended statement that mail semantics remain unchanged. Apply the same wording corrections to the corresponding repeated section.specs/api/agent-inbox-v1.md (1)
406-408: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not leave disabled Agents on bearer-only inbox paths.
The acting-Agent header is attached only to selected calls, while Lines [413-420] explicitly allow a disabled Agent with a valid cookie to read and act on conversations for up to the cookie lifetime. Require the active-Agent check on every Agent-facing inbox endpoint, or revoke/invalidate sessions when disabling an Agent.
🤖 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 `@specs/api/agent-inbox-v1.md` around lines 406 - 408, Update the Agent-facing inbox authentication requirements so disabled Agents cannot access bearer-only paths using an existing valid cookie. Require an active-Agent check on every inbox endpoint, including conversation reads and actions, or explicitly revoke/invalidate all sessions when an Agent is disabled; ensure the documented behavior is consistent across the endpoint rules near the conversation deletion semantics and the requirements in lines 413-420.
🟡 Minor comments (15)
legal/provenance-policy.md-140-145 (1)
140-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the charter attribution in the counsel note.
CHARTER.mdsupports the independent-implementation and defensible-provenance principles, but it does not itself state every listed practice, including license verification at dependency adoption, DCO on every commit, or mandatory human review. Attribute those requirements to the legal guide and this policy, or add the missing commitments to the charter.🤖 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 `@legal/provenance-policy.md` around lines 140 - 145, Correct the attribution in the counsel note’s “Faithful to the legal guide” statement: do not claim that CHARTER.md already commits to every listed practice. Attribute license verification, DCO on every commit, mandatory human review, and the other detailed requirements to the legal guide and this policy, or add those commitments to CHARTER.md before retaining the broader claim.STATUS.md-83-87 (1)
83-87: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the canonical actor and artifact vocabulary.
Line 83 calls AI actors “assistant (AI) actors,” while Line 87 calls the installable artifact a “draft-reply assistant.” Use “Assistants (AI actors)” and “draft-reply Module” so actor identity and distribution artifacts are not conflated.
As per coding guidelines, use
Assistantsfor AI actors andModulesfor installable extension artifacts.🤖 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 `@STATUS.md` around lines 83 - 87, Update the STATUS.md terminology in the Substrate v1 spec and First module scaffold entries: refer to AI actors as “Assistants (AI actors)” and the installable artifact as the “draft-reply Module.” Preserve the existing descriptions and distinguish actor identity from module distribution.Source: Coding guidelines
specs/mail/gmail-connect.md-3-8 (1)
3-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the incomplete introductory sentence.
Line 3 currently reads “The write-side ... deliberately stubbed”; add the missing verb (“is”) so the opening contract is readable and unambiguous.
🤖 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 `@specs/mail/gmail-connect.md` around lines 3 - 8, Update the introductory sentence in the status paragraph to include the missing “is” after “The write-side,” while preserving the surrounding wording and contract description.specs/modules/substrate-v1.md-8-11 (1)
8-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the prescribed Module terminology.
The changed phrase
plugin-exception textviolates the repository vocabulary rule. Rename it to the actual API/module-exception terminology, or use the exact legal phrase “plugin exception” only where that legal exception is intended.🤖 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 `@specs/modules/substrate-v1.md` around lines 8 - 11, Update the wording in the surrounding module specification text to use the repository’s prescribed Module/API terminology: replace “plugin-exception text” with the established module-exception term, or with the exact phrase “plugin exception” only when referring to the legal exception. Preserve the intended meaning of the §7 drafting reference.Source: Coding guidelines
specs/deploy/gmail-inbound-runbook.md-184-185 (1)
184-185: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRepair malformed runbook entries.
The changed text contains broken fragments: “also delivers webhooks, :”, “'s live proof”, and an alert description beginning with “: a webhook endpoint”. Rewrite these lines so the deployment and alerting instructions are actionable.
Also applies to: 263-265, 306-308
🤖 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 `@specs/deploy/gmail-inbound-runbook.md` around lines 184 - 185, Repair the malformed runbook prose in the deployment and alerting sections, including the fragments around WEBHOOK_DELIVERY_TOPIC, “'s live proof,” and “: a webhook endpoint” at the referenced entries. Rewrite each affected line into clear, actionable instructions while preserving the intended deployment and alerting meaning.specs/mail/gmail-push.md-264-275 (1)
264-275: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRepair malformed failure-handling prose.
The changed text contains incomplete fragments such as “predates 's token layer” and the broken OAuth/connect arrow sentence. Rewrite these before relying on the section as an operational contract.
Also applies to: 283-285
🤖 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 `@specs/mail/gmail-push.md` around lines 264 - 275, Repair the failure-handling section prose around the “token layer owns needs_reconnect” heading, including the incomplete “predates 's token layer” fragment and broken OAuth/connect arrow sentence. Preserve the documented behavior: invalid_grant from getAccessToken marks needs-reconnect, generic watch failures remain transient, and failures are isolated per mailbox.specs/mail/inbound-ingestion.md-42-52 (1)
42-52: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRepair incomplete acceptance and contract prose.
The changed text contains fragments such as “changes the seam,” “implements this,” and “PRE--fix failure mode,” plus a broken acceptance sentence after “no orphaned/duplicate conversation.” These should be rewritten so the provider and retry contracts are unambiguous.
Also applies to: 87-100, 270-282, 347-350
🤖 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 `@specs/mail/inbound-ingestion.md` around lines 42 - 52, The inbound-ingestion contract prose remains incomplete and contains placeholder fragments and a broken acceptance sentence. Rewrite the affected sections around the provider interface, retry behavior, and acceptance criteria so they clearly define provider responsibilities, engine responsibilities, idempotency, and the required no-orphaned/duplicate-conversation outcome; remove fragments such as “changes the seam,” “implements this,” and “PRE--fix failure mode,” while preserving the intended invariant that parsing and attachment ownership remain in the engine.specs/mail/gmail-connect.md-242-243 (1)
242-243: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRepair the malformed live-proof sentences.
The changed text contains broken fragments (
is 'sand( territory)). Rewrite these sentences before publishing the operator-facing specification.Also applies to: 359-361
🤖 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 `@specs/mail/gmail-connect.md` around lines 242 - 243, Rewrite the malformed live-proof sentences in the operator-facing specification, including both the changed passage and the corresponding passage at the other referenced location. Remove fragments such as “is 's” and “( territory)” while preserving the intended contrast between a genuine consent/watch/push/reply proof and the fake-backed suite.specs/modules/marketplace-v1.md-176-176 (1)
176-176: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the dangling possessives.
These changed lines contain incomplete fragments such as
('s,gate 's, andreusing 's. Restore the removed owner noun or rewrite each sentence without a possessive before publishing the specification.Also applies to: 601-601, 704-704
🤖 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 `@specs/modules/marketplace-v1.md` at line 176, Fix the incomplete possessive fragments in the specification, including “('s”, “gate 's”, and “reusing 's”, at the referenced occurrences. Restore the intended owner noun or rewrite each sentence to remove the possessive while preserving the original meaning.specs/ui/admin-ia.md-34-34 (1)
34-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the incomplete status copy.
Line 34 should say what is shipping (for example, “shipping” or “shipping incrementally”), and line 47 should read “this spec §11” rather than “spec §11”.
Also applies to: 47-47
🤖 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 `@specs/ui/admin-ia.md` at line 34, Update the Helpthread status copy to explicitly state what is shipping, such as “shipping” or “shipping incrementally,” while preserving the existing Profile, Permissions, and Notifications details. Also revise the cross-reference on line 47 to read “this spec §11” instead of “spec §11”.specs/modules/marketplace-v1.md-296-296 (1)
296-296: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse the catalog name here.
clusteris defined as the catalog cluster name, but this example emits the slugai-automation. Pick one representation and use it consistently so consumers can match clusters reliably.🤖 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 `@specs/modules/marketplace-v1.md` at line 296, Update the cluster value in the marketplace example to use the catalog’s defined cluster name rather than the ai-automation slug, and keep the representation consistent with the catalog and other examples.specs/auth/agents-and-auth.md-514-522 (1)
514-522: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClarify that grants ship even though visibility enforcement is deferred.
Line [520] says “No per-Agent mailbox scoping,” but Lines [156-187] require managed grants, auto-grant-on-create behavior, and Permissions endpoints now. Change this to “No conversation-visibility enforcement yet” so implementers do not omit the required grant model.
🤖 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 `@specs/auth/agents-and-auth.md` around lines 514 - 522, Update the scope statement near the “No per-Agent mailbox scoping” bullet to say that conversation-visibility enforcement is deferred, while explicitly preserving the required managed grants, auto-grant-on-create behavior, and Permissions endpoints defined earlier in the specification.specs/api/agent-inbox-v1.md-581-586 (1)
581-586: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the attachment-scope sentence.
Line [581] says “( wired the READ side...” without a subject. Clarify which layer provides attachment reads and that outbound attachment upload remains unsupported.
🤖 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 `@specs/api/agent-inbox-v1.md` around lines 581 - 586, Update the attachment-scope statement near ThreadView.attachments to explicitly identify the layer responsible for inbound attachment reads, while clearly retaining that Agents cannot upload attachments on outbound replies. Keep the surrounding framework-agnostic guidance unchanged.specs/api/agent-inbox-v1.md-387-392 (1)
387-392: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the note-boundary test requirement actionable.
Line [392] ends with “adds a test” without naming the implementation or assertion. State the exact invariant—no reply token, outbox row, or delivery-worker involvement—and identify where it is tested.
🤖 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 `@specs/api/agent-inbox-v1.md` around lines 387 - 392, Update the POST /api/v1/conversations/{id}/notes specification to state that its boundary test must assert no reply token is minted, no outbox row is created, and no delivery-worker job or invocation occurs; identify the specific test name or test location where these assertions are implemented.specs/api/agent-inbox-v1.md-448-451 (1)
448-451: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRepair the incomplete open-tracking contract text.
Lines [448-451] end with “pinned by)” and leave “must prove” without an actor; Line [461] likewise ends with “pinned by against that requirement.” Restore the missing references and subject so the required configuration and token-format acceptance evidence are unambiguous.
Also applies to: 461-461
🤖 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 `@specs/api/agent-inbox-v1.md` around lines 448 - 451, Repair the open-tracking contract wording in the affected specification sections: complete the sentence after “pinned by” with the responsible actor and subject, and fix the corresponding sentence at the later acceptance requirement. Explicitly identify the required configuration and token-format acceptance evidence while preserving the stated default behavior and HTML-only change constraint.
🧹 Nitpick comments (1)
specs/auth/agents-and-auth.md (1)
216-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
module, notplugin, for the extension boundary.Line [217] calls the not-yet-built mechanism a “plugin loader,” which violates the repository vocabulary rule. Use “module loader” or “module discovery mechanism.”
🤖 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 `@specs/auth/agents-and-auth.md` around lines 216 - 217, Update the terminology in the paragraph around the module-API exception to replace “plugin loader” with “module loader” or “module discovery mechanism,” preserving the existing meaning and repository vocabulary.Source: Coding guidelines
🤖 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 `@GOVERNANCE.md`:
- Around line 17-22: Align the ownership wording across GOVERNANCE.md (lines
17-22) and legal/trademark-policy.md (lines 162-165): update the governance note
about Resonant IQ, Inc. stewarding the Helpthread name and marks, and revise the
trademark-policy cross-reference so both documents state the same legal position
without relying on an unsupported claim in legal/README.md.
In `@legal/module-commercial-license.md`:
- Around line 22-31: Update the Module rights boundary statement to explicitly
include build-time composition, including Modules compiled into the Helpthread
core, alongside documented interfaces and out-of-process integration. Preserve
the existing clarification that the License does not restrict rights in the
AGPL-licensed core.
In `@specs/api/agent-inbox-v1.md`:
- Around line 23-33: Update the rollout note in the document introduction to
identify both breaking increments: the status-model and list-filter changes, and
the mandatory Idempotency-Key requirement described in the write-path section.
State that backend and UI adoption of both changes must be coordinated in the
rollout guidance, preserving the existing sequencing context.
In `@specs/auth/agents-and-auth.md`:
- Around line 486-491: Define concrete shared rate-limit and resource-abuse
controls for public password authentication in the rate-limiting section of
specs/auth/agents-and-auth.md (lines 486-491), including limits for password
verification. Apply equivalent limits to passkey options and verification in
specs/auth/passkeys.md (lines 988-993), ensuring both specifications document
the required enforcement before release.
- Around line 406-408: Update the documented web inbox authorization flow so
disabled-Agent validation applies to every Agent-facing operation, including the
bearer-only conversation operations described near the api.ts request behavior.
Do not leave those operations usable with a stale disabled session; either
require the disabled-Agent assertion on each operation or specify immediate
session revocation.
In `@specs/deploy/gmail-inbound-runbook.md`:
- Around line 40-46: Resolve the queue-provider inconsistency by confirming the
actual queue adapter, then align both contracts to it: update the architecture
description at specs/deploy/gmail-inbound-runbook.md lines 40-46 and the
QueueProvider/Vercel Queues wording at specs/mail/gmail-push.md lines 71-79. If
the adapter is PG-backed, retain the durable INSERT and Vercel Cron drain flow
and remove conflicting Vercel Queues language; otherwise replace the diagram
with the selected implementation and update QueueProvider accordingly.
In `@specs/modules/marketplace-v1.md`:
- Around line 130-134: Update the refund transition documentation around the
`charge.refunded`/`refund.created` mapping so partial refunds do not produce
`refunded`; require a full refund or aggregated refund amount before revoking
entitlement, while preserving the existing lost-dispute behavior. Add a
partial-refund scenario to the dogfood plan.
- Line 193: Update the SQL UPDATE statement to call PostgreSQL’s timestamp
function as now() when assigning revealed_at, while preserving the existing
license_keys fields and WHERE conditions.
---
Outside diff comments:
In `@specs/api/agent-inbox-v1.md`:
- Around line 406-408: Update the Agent-facing inbox authentication requirements
so disabled Agents cannot access bearer-only paths using an existing valid
cookie. Require an active-Agent check on every inbox endpoint, including
conversation reads and actions, or explicitly revoke/invalidate all sessions
when an Agent is disabled; ensure the documented behavior is consistent across
the endpoint rules near the conversation deletion semantics and the requirements
in lines 413-420.
In `@specs/mail/sending.md`:
- Around line 178-200: In the affected documentation passages, restore the
broken identifier as SendReplyInput.from and remove the extra spacing in “The
fix is...”. Ensure the surrounding sentence clearly and accurately states the
self-echo guard contract for implementers, preserving the existing technical
meaning.
In `@specs/modules/substrate-v1.md`:
- Around line 106-112: Rewrite the malformed prose in the schema and
delivery-status contract: replace “A draft is inert rows until approval” with
grammatically correct wording stating that drafts remain inert until approval,
and correct the malformed “( semantics unchanged” parenthetical while preserving
the intended statement that mail semantics remain unchanged. Apply the same
wording corrections to the corresponding repeated section.
---
Minor comments:
In `@legal/provenance-policy.md`:
- Around line 140-145: Correct the attribution in the counsel note’s “Faithful
to the legal guide” statement: do not claim that CHARTER.md already commits to
every listed practice. Attribute license verification, DCO on every commit,
mandatory human review, and the other detailed requirements to the legal guide
and this policy, or add those commitments to CHARTER.md before retaining the
broader claim.
In `@specs/api/agent-inbox-v1.md`:
- Around line 581-586: Update the attachment-scope statement near
ThreadView.attachments to explicitly identify the layer responsible for inbound
attachment reads, while clearly retaining that Agents cannot upload attachments
on outbound replies. Keep the surrounding framework-agnostic guidance unchanged.
- Around line 387-392: Update the POST /api/v1/conversations/{id}/notes
specification to state that its boundary test must assert no reply token is
minted, no outbox row is created, and no delivery-worker job or invocation
occurs; identify the specific test name or test location where these assertions
are implemented.
- Around line 448-451: Repair the open-tracking contract wording in the affected
specification sections: complete the sentence after “pinned by” with the
responsible actor and subject, and fix the corresponding sentence at the later
acceptance requirement. Explicitly identify the required configuration and
token-format acceptance evidence while preserving the stated default behavior
and HTML-only change constraint.
In `@specs/auth/agents-and-auth.md`:
- Around line 514-522: Update the scope statement near the “No per-Agent mailbox
scoping” bullet to say that conversation-visibility enforcement is deferred,
while explicitly preserving the required managed grants, auto-grant-on-create
behavior, and Permissions endpoints defined earlier in the specification.
In `@specs/deploy/gmail-inbound-runbook.md`:
- Around line 184-185: Repair the malformed runbook prose in the deployment and
alerting sections, including the fragments around WEBHOOK_DELIVERY_TOPIC, “'s
live proof,” and “: a webhook endpoint” at the referenced entries. Rewrite each
affected line into clear, actionable instructions while preserving the intended
deployment and alerting meaning.
In `@specs/mail/gmail-connect.md`:
- Around line 3-8: Update the introductory sentence in the status paragraph to
include the missing “is” after “The write-side,” while preserving the
surrounding wording and contract description.
- Around line 242-243: Rewrite the malformed live-proof sentences in the
operator-facing specification, including both the changed passage and the
corresponding passage at the other referenced location. Remove fragments such as
“is 's” and “( territory)” while preserving the intended contrast between a
genuine consent/watch/push/reply proof and the fake-backed suite.
In `@specs/mail/gmail-push.md`:
- Around line 264-275: Repair the failure-handling section prose around the
“token layer owns needs_reconnect” heading, including the incomplete “predates
's token layer” fragment and broken OAuth/connect arrow sentence. Preserve the
documented behavior: invalid_grant from getAccessToken marks needs-reconnect,
generic watch failures remain transient, and failures are isolated per mailbox.
In `@specs/mail/inbound-ingestion.md`:
- Around line 42-52: The inbound-ingestion contract prose remains incomplete and
contains placeholder fragments and a broken acceptance sentence. Rewrite the
affected sections around the provider interface, retry behavior, and acceptance
criteria so they clearly define provider responsibilities, engine
responsibilities, idempotency, and the required
no-orphaned/duplicate-conversation outcome; remove fragments such as “changes
the seam,” “implements this,” and “PRE--fix failure mode,” while preserving the
intended invariant that parsing and attachment ownership remain in the engine.
In `@specs/modules/marketplace-v1.md`:
- Line 176: Fix the incomplete possessive fragments in the specification,
including “('s”, “gate 's”, and “reusing 's”, at the referenced occurrences.
Restore the intended owner noun or rewrite each sentence to remove the
possessive while preserving the original meaning.
- Line 296: Update the cluster value in the marketplace example to use the
catalog’s defined cluster name rather than the ai-automation slug, and keep the
representation consistent with the catalog and other examples.
In `@specs/modules/substrate-v1.md`:
- Around line 8-11: Update the wording in the surrounding module specification
text to use the repository’s prescribed Module/API terminology: replace
“plugin-exception text” with the established module-exception term, or with the
exact phrase “plugin exception” only when referring to the legal exception.
Preserve the intended meaning of the §7 drafting reference.
In `@specs/ui/admin-ia.md`:
- Line 34: Update the Helpthread status copy to explicitly state what is
shipping, such as “shipping” or “shipping incrementally,” while preserving the
existing Profile, Permissions, and Notifications details. Also revise the
cross-reference on line 47 to read “this spec §11” instead of “spec §11”.
In `@STATUS.md`:
- Around line 83-87: Update the STATUS.md terminology in the Substrate v1 spec
and First module scaffold entries: refer to AI actors as “Assistants (AI
actors)” and the installable artifact as the “draft-reply Module.” Preserve the
existing descriptions and distinguish actor identity from module distribution.
---
Nitpick comments:
In `@specs/auth/agents-and-auth.md`:
- Around line 216-217: Update the terminology in the paragraph around the
module-API exception to replace “plugin loader” with “module loader” or “module
discovery mechanism,” preserving the existing meaning and repository vocabulary.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f9979d60-456f-4df5-a612-c388442eff53
📒 Files selected for processing (33)
CHARTER.mdCLAUDE.mdCONTRIBUTING.mdGOVERNANCE.mdREADME.mdSTATUS.mddocs/architecture/README.mddocs/decisions/README.mddocs/history/CHARTER-v1.mddocs/history/README.mdlegal/README.mdlegal/module-api-exception.mdlegal/module-commercial-license.mdlegal/provenance-policy.mdlegal/trademark-policy.mdpackage.jsonspecs/api/agent-inbox-v1.mdspecs/auth/agents-and-auth.mdspecs/auth/passkeys.mdspecs/deploy/gmail-inbound-runbook.mdspecs/mail/gmail-connect.mdspecs/mail/gmail-push.mdspecs/mail/inbound-ingestion.mdspecs/mail/mailbox-connection.mdspecs/mail/sending.mdspecs/mail/threading.mdspecs/modules/catalog.mdspecs/modules/marketplace-v1.mdspecs/modules/substrate-v1.mdspecs/store/conversations.mdspecs/ui/admin-ia.mdsrc/providers/README.mdweb/README.md
| Resonant IQ, Inc. stewards: | ||
|
|
||
| - the Helpthread name and marks; | ||
| - the official repository and distribution channels; | ||
| - the official marketplace; and | ||
| - its own contributions. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== GOVERNANCE.md ==\n'
sed -n '1,80p' GOVERNANCE.md | cat -n
printf '\n== legal/trademark-policy.md ==\n'
sed -n '140,190p' legal/trademark-policy.md | cat -n
printf '\n== legal/README.md ==\n'
sed -n '1,220p' legal/README.md | cat -nRepository: Helpthread/helpthread
Length of output: 6960
Align the stewardship and ownership wording
GOVERNANCE.md says Resonant IQ, Inc. “stewsards” the Helpthread name and marks, but legal/trademark-policy.md still ties those marks to legal/README.md as an ownership claim, and that guide does not make that claim. Update the note and cross-reference so both files use the same legal position.
📍 Affects 2 files
GOVERNANCE.md#L17-L22(this comment)legal/trademark-policy.md#L162-L165
🤖 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 `@GOVERNANCE.md` around lines 17 - 22, Align the ownership wording across
GOVERNANCE.md (lines 17-22) and legal/trademark-policy.md (lines 162-165):
update the governance note about Resonant IQ, Inc. stewarding the Helpthread
name and marks, and revise the trademark-policy cross-reference so both
documents state the same legal position without relying on an unsupported claim
in legal/README.md.
| A **"Module"** is a first-party Helpthread extension that Resonant IQ distributes for a | ||
| fee through the official Helpthread marketplace, delivered to you as **Source** — a | ||
| source-code tarball, not a compiled binary. The **"Source"** is the complete tarball | ||
| contents of a Module release as published. This License governs your use of the Module | ||
| and its Source. It does **not** govern the Helpthread core, which is separately licensed | ||
| under the GNU Affero General Public License, version 3.0 (**"AGPL-3.0"**), together with | ||
| any additional permissions the core's LICENSE file carries (including, once adopted, the | ||
| Helpthread Module API Exception), nor any third-party dependency, which each carry their | ||
| own licenses. The Source may include third-party materials under their own permissive | ||
| licenses; their notices travel with the Source as delivered. Your rights in the Module are | ||
| under the GNU Affero General Public License, version 3.0 (**"AGPL-3.0"**), nor any | ||
| third-party dependency, which each carry their own licenses. Your rights in the Module are | ||
| exercised through the documented module interfaces and out-of-process integration; nothing | ||
| in this License restricts, modifies, or replaces any right you have in the AGPL-licensed | ||
| core under the AGPL-3.0. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files CHARTER.md legal/module-api-exception.md legal/module-commercial-license.md
printf '\n== CHARTER.md (relevant lines) ==\n'
rg -n -C 3 'build-time|module|out-of-process|composition|compile|compile-time|in-process' CHARTER.md || true
printf '\n== legal/module-api-exception.md (relevant lines) ==\n'
rg -n -C 3 'build-time|module|out-of-process|composition|compile|compile-time|in-process' legal/module-api-exception.md || true
printf '\n== legal/module-commercial-license.md (relevant lines) ==\n'
rg -n -C 3 'Module|Source|out-of-process|interfaces|build-time|in-process' legal/module-commercial-license.md || trueRepository: Helpthread/helpthread
Length of output: 17497
Clarify that build-time module composition is included.
This reads as limiting Module rights to documented interfaces and out-of-process integration, but the charter and Module API exception also cover build-time modules compiled into the core. State that the boundary includes build-time composition as well.
🤖 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 `@legal/module-commercial-license.md` around lines 22 - 31, Update the Module
rights boundary statement to explicitly include build-time composition,
including Modules compiled into the Helpthread core, alongside documented
interfaces and out-of-process integration. Preserve the existing clarification
that the License does not restrict rights in the AGPL-licensed core.
| This document covers the whole v1 surface. The read paths and conventions are in §3; | ||
| write paths are in §4; §4a includes send idempotency and appends the reply's own minted | ||
| id to `References` because a provider—Gmail, confirmed live—can rewrite `Message-ID` | ||
| on send (see threading.md §2a). | ||
| The v1.1 additions include the status model, `preview` and `number`, notes, tags, | ||
| deletion, assignment, and open tracking. | ||
|
|
||
| Rollout note: **HT-26 is the one BREAKING increment** — existing status values are | ||
| Rollout note: **the status-model change is the one breaking increment** — existing status values are | ||
| renamed and the list filter's meaning changes, so backend and UI adopt it together (a | ||
| coordinated rollout, deliberately first in the sequence; dogfood-only means the | ||
| coordination is a single deploy, per HT-16's same reasoning). Every OTHER addition is | ||
| coordination is a single deploy). Every other addition is |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document all breaking increments in the rollout note.
Line [30-33] says the status-model change is the one breaking increment, but Lines [232-242] make Idempotency-Key mandatory and explicitly call that a breaking change. List both changes and coordinate them in rollout guidance; otherwise a deployment may treat the required header as additive.
🤖 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 `@specs/api/agent-inbox-v1.md` around lines 23 - 33, Update the rollout note in
the document introduction to identify both breaking increments: the status-model
and list-filter changes, and the mandatory Idempotency-Key requirement described
in the write-path section. State that backend and UI adoption of both changes
must be coordinated in the rollout guidance, preserving the existing sequencing
context.
| `X-Helpthread-Agent-Id: <agentId>`. Today `web/src/lib/api.ts`'s `request` reads only server | ||
| env and has no access to the request's session, so this is a **real refactor, specified here, | ||
| not a free carry-over**: `request()` (server-only already) reads and verifies the session | ||
| not a free carry-over**: `request` (server-only already) reads and verifies the session |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Ensure disabled-Agent checks cover all web inbox operations.
The new api.ts behavior attaches identity only to selected calls, while Lines [413-420] knowingly leave bearer-only conversation operations usable with a stale disabled session. This creates an authorization window inconsistent with disabling an Agent; require the assertion on every Agent-facing operation or implement immediate session revocation.
🤖 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 `@specs/auth/agents-and-auth.md` around lines 406 - 408, Update the documented
web inbox authorization flow so disabled-Agent validation applies to every
Agent-facing operation, including the bearer-only conversation operations
described near the api.ts request behavior. Do not leave those operations usable
with a stale disabled session; either require the disabled-Agent assertion on
each operation or specify immediate session revocation.
| - **Session crypto** remains HMAC using Web Crypto on Edge. Middleware still only | ||
| *verifies* the cookie — it never touches the Agent store, so the Edge/`node:crypto` | ||
| constraint holds. | ||
| - **Rate-limiting** remains the HT-51 per-instance gap | ||
| ([HT-53](https://resonantiq.atlassian.net/browse/HT-53)) — now more pressing with multiple | ||
| - **Rate-limiting** remains a per-instance gap | ||
| — now more pressing with multiple | ||
| accounts and a public login. Not solved here; called out. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Close the shared authentication rate-limit gap before release.
Both specifications document public password/passkey authentication without throttling or resource-abuse controls.
specs/auth/agents-and-auth.md#L486-L491: define limits for password verification.specs/auth/passkeys.md#L988-L993: apply equivalent limits to passkey options and verification.
📍 Affects 2 files
specs/auth/agents-and-auth.md#L486-L491(this comment)specs/auth/passkeys.md#L988-L993
🤖 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 `@specs/auth/agents-and-auth.md` around lines 486 - 491, Define concrete shared
rate-limit and resource-abuse controls for public password authentication in the
rate-limiting section of specs/auth/agents-and-auth.md (lines 486-491),
including limits for password verification. Apply equivalent limits to passkey
options and verification in specs/auth/passkeys.md (lines 988-993), ensuring
both specifications document the required enforcement before release.
| Gmail mailbox ──watch──▶ Cloud Pub/Sub topic ──push sub (OIDC JWT)──▶ | ||
| POST /api/v1/inbound/gmail (webhook: verify JWT → enqueue reconcile job → 2xx) | ||
| │ enqueue (durable INSERT into the PG job queue — commits BEFORE the 2xx) | ||
| ▼ | ||
| Vercel Cron ──GET /api/v1/internal/queue/drain (every minute)──▶ drain N jobs: | ||
| reconcile (history.list → messages.get raw) → idempotent ingest → conversation | ||
| Vercel Cron ──GET /api/v1/internal/cron/watch-maintenance (daily)──▶ re-arm watch() + sweep | ||
| Vercel Cron ──GET /api/v1/internal/cron/watch-maintenance (daily)──▶ re-arm watch + sweep |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve the queue-provider contradiction across the mail contracts.
The deployment diagram specifies the dogfood path as a PG-backed queue drained by Vercel Cron, while the Gmail push spec now says to enqueue onto Vercel Queues. These instructions describe different runtime implementations and must be made consistent before release.
specs/deploy/gmail-inbound-runbook.md#L40-L46: retain or replace the PG-backed architecture description based on the actual adapter.specs/mail/gmail-push.md#L71-L79: update theQueueProvider/Vercel Queues wording to match the selected implementation.
📍 Affects 2 files
specs/deploy/gmail-inbound-runbook.md#L40-L46(this comment)specs/mail/gmail-push.md#L71-L79
🤖 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 `@specs/deploy/gmail-inbound-runbook.md` around lines 40 - 46, Resolve the
queue-provider inconsistency by confirming the actual queue adapter, then align
both contracts to it: update the architecture description at
specs/deploy/gmail-inbound-runbook.md lines 40-46 and the QueueProvider/Vercel
Queues wording at specs/mail/gmail-push.md lines 71-79. If the adapter is
PG-backed, retain the durable INSERT and Vercel Cron drain flow and remove
conflicting Vercel Queues language; otherwise replace the diagram with the
selected implementation and update QueueProvider accordingly.
| - **Refund** (`charge.refunded`/`refund.created`, or a *lost* dispute) → `refunded`. | ||
| Terminal for entitlement, explicitly not a fraud label — the buy→download→refund | ||
| path must not let the customer keep both the money and the bits, but it also must | ||
| not brand an ordinary refund request as fraud. A **partial or goodwill refund leaves | ||
| entitlement untouched** (HT-86) — no state change. Running software already deployed | ||
| is untouched either way, per §1's governing constraint. | ||
| not brand an ordinary refund request as fraud. Running software already deployed is | ||
| untouched either way, per §1's governing constraint. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section with line numbers.
sed -n '110,150p' specs/modules/marketplace-v1.md
echo
echo "----"
# Find other refund-related references in the spec.
rg -n "charge\.refunded|refund\.created|refunded|partial refund|partial" specs/modules/marketplace-v1.mdRepository: Helpthread/helpthread
Length of output: 7455
🌐 Web query:
Stripe charge.refundedpartial refund semanticsrefund.created webhook
💡 Result:
When handling partial refunds in Stripe, it is recommended to prioritize the refund.created event over the legacy charge.refunded event for accessing detailed information about the refund [1][2]. Key semantic and technical distinctions: 1. Event Semantics: The charge.refunded event is triggered whenever a charge is refunded, regardless of whether it is a full or partial refund [3][4][5]. However, this event provides the Charge object in its data payload, which does not inherently contain detailed data for the individual refund operation that just occurred [1][2]. 2. Preference for refund.created: Stripe now issues refund.created, refund.updated, and refund.failed events for all types of refunds, including those with a corresponding charge [1][2]. The refund.created event payload contains the specific Refund object [6][7]. Listening to this event allows you to access the refund details directly without needing to make an additional API call to retrieve or expand the Charge object [1][2]. 3. Legacy Handling: Previously, developers were often forced to react to charge.refunded by making a separate API request to Stripe to fetch and expand the Charge object to inspect the status of the refund list [8]. With the introduction of the more consistent refund.* suite of events, this extra step is no longer necessary, as the Refund object itself provides the required information [1][2]. In summary, for any new integrations or to improve existing ones, you should listen for refund.created to obtain complete, immediate details about a partial refund [1][2].
Citations:
- 1: https://docs.stripe.com/changelog/acacia/2024-10-28/refund-webhook-update
- 2: https://docs.stripe.com/changelog/acacia/2024-10-28/refund-webhook-update?locale=en-GB
- 3: https://docs.stripe.com/refunds?dashboard-or-api=dashboard
- 4: https://docs.stripe.com/api/charges?api-version=2026-04-22.dahlia
- 5: https://docs.stripe.com/api/webhook_endpoints/create
- 6: https://docs.stripe.com/api/events/types
- 7: https://docs.stripe.com/api/events/types?api-version=2026-05-27.preview
- 8: https://stackoverflow.com/questions/75897332/stripe-api-2022-11-15-charge-refunds-and-charge-refunded-webhook-event
Don't collapse partial refunds into refunded. charge.refunded can fire for partial refunds, and refund.created carries the refund object directly, so this mapping revokes entitlement on a partial refund too. Gate the transition on a full refund or aggregated refund amount, and add a partial-refund case to the dogfood plan.
🤖 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 `@specs/modules/marketplace-v1.md` around lines 130 - 134, Update the refund
transition documentation around the `charge.refunded`/`refund.created` mapping
so partial refunds do not produce `refunded`; require a full refund or
aggregated refund amount before revoking entitlement, while preserving the
existing lost-dispute behavior. Add a partial-refund scenario to the dogfood
plan.
| the authenticated customer requests that subscription's key, the marketplace mints | ||
| and stores it inside a single **compare-and-set** transaction: | ||
| `UPDATE license_keys SET secret_hash = $hash, revealed_at = now() WHERE id = $id AND | ||
| `UPDATE license_keys SET secret_hash = $hash, revealed_at = now WHERE id = $id AND |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of the spec with line numbers
FILE="specs/modules/marketplace-v1.md"
wc -l "$FILE"
sed -n '175,210p' "$FILE" | cat -n
# Search for other occurrences of the same SQL fragment or revealed_at assignment
rg -n "revealed_at = now|revealed_at = now\(\)|secret_hash = \$hash" specs modules . || trueRepository: Helpthread/helpthread
Length of output: 3370
🌐 Web query:
PostgreSQL zero-argument function call syntax now without parentheses
💡 Result:
In PostgreSQL, the standard syntax for calling a function requires the inclusion of parentheses, even when the function takes zero arguments [1][2]. The documentation explicitly states that a function call must consist of the function name followed by its argument list enclosed in parentheses [2]. Attempting to call a user-defined function without parentheses typically results in a syntax error [1][3]. There is a distinction in PostgreSQL between user-defined functions and certain built-in system information functions [4]. Some system-level identifiers—such as CURRENT_USER, SESSION_USER, and CURRENT_SCHEMA—have special syntactic status and are called without parentheses [4]. However, this behavior is a specific property of those internal SQL keywords and does not apply to standard user-defined functions or stored procedures [4][1]. While there have been technical discussions within the PostgreSQL community regarding allowing the omission of argument lists for functions in specific DDL contexts (such as DROP FUNCTION) when the name is unique, this has not been extended to general function invocation within SQL queries [5]. As of July 2026, the requirement to use parentheses for all user-defined function calls remains in place [2][6].
Citations:
- 1: https://stackoverflow.com/questions/69009198/is-it-possible-to-call-a-function-procedure-in-postgres-without-braces
- 2: https://www.postgresql.org/docs/18/sql-expressions.html
- 3: https://www.postgresql.org/docs/18/xfunc-sql.html
- 4: https://www.postgresql.org/docs/current/functions-info.html
- 5: https://www.mail-archive.com/pgsql-hackers@postgresql.org/msg306508.html
- 6: https://www.postgresql.org/docs/19/xfunc-sql.html
Use now() in the UPDATE statement.
now needs parentheses in PostgreSQL; as written, this UPDATE will fail instead of storing the timestamp.
Proposed fix
- UPDATE license_keys SET secret_hash = $hash, revealed_at = now WHERE id = $id AND
+ UPDATE license_keys SET secret_hash = $hash, revealed_at = now() WHERE id = $id AND📝 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.
| `UPDATE license_keys SET secret_hash = $hash, revealed_at = now WHERE id = $id AND | |
| `UPDATE license_keys SET secret_hash = $hash, revealed_at = now() WHERE id = $id AND |
🤖 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 `@specs/modules/marketplace-v1.md` at line 193, Update the SQL UPDATE statement
to call PostgreSQL’s timestamp function as now() when assigning revealed_at,
while preserving the existing license_keys fields and WHERE conditions.
1 participant
What changed
docs/history/.Why
The previous charter mixed product identity, current architecture, legal decisions, amendments, and implementation history. This revision gives contributors a durable decision framework while keeping changing details in maintained documents that can evolve independently.
Validation
mainand reconciled the overlapping managed-hosting documentation.git diff --checkpasses.@simplewebauthn/serverdependency/type declarations and stops there.src/providers/adapters/gmail/sender.test.ts.Review note
This is intentionally a draft PR. The most important review targets are
CHARTER.md,README.md,GOVERNANCE.md,docs/decisions/README.md,specs/modules/catalog.md, andspecs/modules/marketplace-v1.md.Summary by CodeRabbit