[SelerityMichael]

Summary

Fixes #22. accelo_set_profile_value returned status: ok but silently failed to persist the value for a company hyperlink profile field — a false-positive write that automation would trust and move on from.

Root cause

The write was driven through the parent object's update endpoint — PUT /{object}/{id} with the value keyed as profile.{profile_field_id}. Per the Accelo API reference, that profile.{id} form is only the create-company shortcut (documented for POST /companies), not a general profile-value write. For a hyperlink field the parent PUT is accepted (the company updates and echoes back), but the profile.{id} value is never applied — and ok is still returned. The previous docstring's claim that the dedicated /profiles/fields/{id} POST path is "rejected by Accelo" was incorrect and was the source of the bug.

What changed (src/accelo_mcp/tools/profiles.py)

• Correct endpoint. The write now goes to the documented dedicated path POST /{object}/{id}/profiles/fields/{field_id} ("Set a Profile Field Value").
• Typed body by field_type. The field's type is resolved via GET /{object}/profiles/fields, then the body is shaped accordingly:
  • integer → value_int
  • date / date_time → value_date
  • lookup → value_id + value_type (new optional value_type parameter)
  • everything else, including hyperlink, text, decimal, currency, select, multi_select → value (string)
• Read-back verify (safety net). After writing, the tool re-reads via GET /{object}/{id}/profiles/values?_fields=_ALL and confirms the value actually changed. A missing row or a mismatch returns a verify_failed error instead of ok — so the tool can never report a silent no-op as success again, regardless of field type or future API quirks.
• Docstring rewritten; the incorrect "endpoint not recognized" claim removed. Australian English.

Testing

Tests mock the Accelo HTTP layer with respx (tests/test_tools/test_profiles.py):

• Hyperlink happy path — asserts the dedicated POST is used (not the parent PUT) with value=<url> in the body.
• Silent-no-op regression guard — write returns ok but the read-back is unchanged (and a no-row variant); the tool returns verify_failed, not ok. This is the core proof the false positive can't recur.
• Lookup (value_id + value_type), integer (value_int), and the structured-error case.

8 profile tests pass; ruff check and ruff format --check both clean.

Release

No version bump in this PR — the release is batched. A single consolidated version bump + release note will be cut once #16, #22 and #23 have all merged.

Closes #22.

[amazon-q-developer]

Review Summary

The core bug fix is sound: replacing the PUT /{object}/{id} + profile.{id} approach with the dedicated POST /{object}/{id}/profiles/fields/{field_id} endpoint, and adding a read-back verification step, correctly addresses the silent no-op for hyperlink fields. The test coverage for the regression scenarios is thorough and well-structured.

There are four blocking issues to address before merge:

1. int(value) crash (line 138, profiles.py) — ValueError is not caught by except AcceloAPIError, so a non-integer string passed to an integer field will raise an unhandled exception instead of returning a structured error.

2. int(value) crash in _value_matches (line 19, profiles.py) — The same ValueError exposure exists in the read-back verification helper; it is outside the try/except AcceloAPIError block entirely.

3. None serialised as "None" for lookup value_type (line 142, profiles.py) — When value_type is omitted (defaults to None), it is still included in the form-encoded body as the literal string "None", which is an invalid Accelo API payload. This should be excluded when None and the field type is lookup (or an explicit early error should be raised).

4. No pagination for field-type resolution (line 120, profiles.py) — The GET /{object_type}/profiles/fields call uses no _limit or pagination, so the target field will silently produce field_not_found if it lives beyond the default page. The test mock always returns a single item so this is not caught by existing tests.

---

You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

[amazon-q-developer]

🛑 Crash Risk: int(value) raises ValueError if value is not a valid integer string (e.g. "abc", "3.5"), but this is not caught by the surrounding except AcceloAPIError handler. A non-integer value passed to an integer field will propagate as an unhandled exception rather than returning a structured error. Wrap the coercion in a try/except ValueError and return an error dict with a meaningful status (e.g. "invalid_value") to match the tool's existing error-response contract.

[amazon-q-developer]

🛑 Logic Error: The lookup field body includes "value_type": value_type even when value_type is None (the default). Sending value_type=None in the form-encoded body will serialise as the string "None", which is an invalid payload for the Accelo API and may cause the write to fail or produce unexpected behaviour. Add a guard to only include value_type in the body when it is not None, and consider returning an early error when it is absent for a lookup field.

[amazon-q-developer]

🛑 Logic Error: The _value_matches helper for the integer case also calls int(value) without guarding against ValueError. If value is not a valid integer string, the read-back verification step will raise an unhandled exception instead of returning verify_failed. This path is reachable if the Accelo API accepted the write for a field that was stored with value_int despite a non-integer value being submitted.

[amazon-q-developer]

🛑 Logic Error: The field-type resolution at line 120 fetches /{object_type}/profiles/fields without any pagination parameters. If the object type has more profile fields than fit in the default page size, the target profile_field_id may fall on a subsequent page and never be found, causing field_not_found to be returned even though the field exists. The _fields_route test helper always returns a single-item list so this gap is not covered by the test suite.

[SelerityMichael]

Thanks — all four findings were valid and are fixed in 49a08c6:

1. int(value) crash on write — the integer branch now guards int(value) and returns a structured invalid_value error before any write, instead of letting ValueError escape the except AcceloAPIError handler.
2. int(value) in _value_matches — guarded too; the helper returns False (non-match) on a non-integer rather than raising, so it can never throw.
3. lookup value_type=None → "None" — a lookup field now requires value_type; a missing one returns a value_type_required error, so a None is never form-encoded into the body.
4. field resolution pagination — the field-type lookup now requests _limit=100 (Accelo's documented max), so a field beyond the default first page resolves instead of falsely returning field_not_found.

Four new tests cover each path (parsed-value assertions, no URL substrings): non-integer rejected with no write POST, lookup-missing-value_type rejected with no write POST, lookup happy path asserting exact value_id/value_type, and _limit=100 proven. 12 profile tests pass; ruff clean.

(The CodeQL annotation still shown at test_profiles.py:93 is stale — that line was fixed in fb83101 to use a parse_qs equality assertion, and CodeQL passes on the PR.)